5201 zio interceptor

[cheleb]

Provides ZIO OpenTelementry Tracing interceptor.

ZIO OpenTelemetry example with OpenTelemetry setup is provided with:

• Trace
• Logging
• Metric

As ZIO OpenTelemetry provides them.

[adamw]

Thanks - below a preliminary review by Claude:

  Blockers / major correctness

  1. The request span is never made the current context — child spans won't nest under it. ZIOpenTelemetryTracing.scala:75-88 uses tracing.extractSpanUnsafe(...) (returns (Span, finalizer); does not install context) and
  runs requestHandler in a plain flatMap. It never calls inSpan/spanScoped. Verified: the reference interceptors both scope the context (opentelemetry uses span.makeCurrent(); otel4s uses build.use), and the PR's own
  example expects … @@ span("hello-logic") to be a child. As written, user-logic spans become detached roots — defeating the interceptor's main purpose. No test covers parent/child nesting. Fix: wrap requestHandler(...)
  in tracing.inSpan(span, …).
  2. Every unmatched request is marked ERROR / error.type=400. :115-118 treats RequestResult.Failure (which includes the normal "no endpoint matched, try the next route" case) as an error. Both reference interceptors
  deliberately ignore Failure. This will pollute traces with false errors.
  3. Streaming response path is the production path for zio-http, and it's both incomplete and untested. finalizeSpan (:132-145) only sets ERROR status on the non-stream branch — a streamed 4xx/5xx never gets span-error
  status. The whole stream branch + handleStreamError has zero tests (the spec uses NoStreams everywhere).
  4. config.spanName is dead config. Documented and defaulted, but never read — :79 hardcodes request.showShort. (The sibling module does use config.spanName.) Either wire it in or remove it.
  5. finalize runs twice on error responses. spanError ends with *> finalize (:224), and the non-stream branch does …spanError(…) *> finalize (:142-144). Harmless today (span.end() is idempotent) but signals muddled
  lifecycle ownership.

  Structural (the biggest-picture issue)

  6. Scope creep: the module bundles application bootstrap that every other observability module deliberately leaves to the user. opentelemetry-metrics/-tracing ship only interceptor + config and dependsOn(serverCore).
  This PR additionally hardcodes exporter type (OTLP gRPC), processor, reader interval, env-var names, a ZIOApp base class, and 184 lines of hand-rolled Noop Meter/Tracing. The interceptor already takes a ready Tracing,
  so none of it is needed to function. Recommendation: keep interceptor + config in the module; move the bootstrap to the examples (or a separate *-bootstrap artifact).
  7. Backend coupling. The "generic" interceptor imports sttp.tapir.server.ziohttp.ZioStreamHttpResponseBody (:25,134,140, plus an asInstanceOf[B]), tying it to the zio-http backend. The build dependsOn(zio,
  zioHttpServer, opentelemetryMetrics) then a second dependsOn(serverCore % CompileAndTest) — the double dependsOn is inconsistent, and the metrics/zio-http deps come only from the scaffolding. Target should be
  dependsOn(zio, serverCore % CompileAndTest).

  Minor / nits

  - Versions.scala: openTelemetrySemconvVersion 1.41.1 → 1.41.0 — an unexplained downgrade; looks accidental.
  - Examples shipped as .skip — excluded from compilation, so unverified, and they reference consoleLogLayer which doesn't exist (the method is baseLogLayer). A real compiled example would have caught it.
  - Module possibly built for Scala 2.12 (scala2And3Versions) while otel4s modules use 2.13+3 — verify zio-opentelemetry 3.1.15 publishes for 2.12.
  - Side-effecting OTel calls wrapped in ZIO.succeed (should be ZIO.attempt, else a thrown exception becomes a defect that skips finalize); MutableMap carrier builder; handleStreamError four arms collapse to one; doc
  typos (Otel4z, [[ZIOpenTelemetry]], oltp); file/type name mismatches (OtelEndpoint.scala → object OtlpEndpoint, Logger.scala → trait Logging).

  Bottom line

  The interceptor concept is sound and matches an established tapir pattern, but it shouldn't merge as-is. Two things must be fixed for it to do its job: (1) make the span current (inSpan) so child spans correlate, and
  (2) stop flagging unmatched requests as errors. Structurally, the PR should be slimmed to interceptor + config (move the SDK bootstrap to examples) and decoupled from zio-http. Test coverage needs a child-span/nesting
  test and streaming tests. The semconv downgrade and .skip examples should be corrected.

[cheleb]

1. The request span is never made the current context — child spans won't nest under it. ZIOpenTelemetryTracing.scala:75-88 uses tracing.extractSpanUnsafe(...) (returns (Span, finalizer); does not install context) and
   runs requestHandler in a plain flatMap. It never calls inSpan/spanScoped. Verified: the reference interceptors both scope the context (opentelemetry uses span.makeCurrent(); otel4s uses build.use), and the PR's own
   example expects … @@ span("hello-logic") to be a child. As written, user-logic spans become detached roots — defeating the interceptor's main purpose. No test covers parent/child nesting. Fix: wrap requestHandler(...)
   in tracing.inSpan(span, …).

But
extractSpanUnsafe doc says:

"Extracts the span from carrier C and unsafely set its child span with name 'spanName' as the current span."

And AFAIU "hello-logic" span is attached the tapir handler parent span :-/

I am missing something on this point ?

[adamw]

@cheleb Yes you're right, sorry. Claude also now agrees:

  Re-review of the ZIO OpenTelemetry tracing interceptor — thanks for the iterations, the latest commits resolved the important items.

  First, a correction on my side. My earlier "blocker" — "the request span is never made current, so child spans won't nest" — was wrong, and you were right to push back. I checked the actual zio-telemetry 3.1.16
  source: extractSpanUnsafe does ctxStorage.getAndSet(updatedCtx), i.e. it does install the span as the current context (the returned finalize restores it). "Unsafe" only means you manage the lifecycle yourself. My
  review had only inspected the no-op NoopTracing stub and inferred from the …Unsafe naming. Apologies — please disregard that point entirely. Your new "child span can be created with correct parent context" test nails
  it down nicely.

  Resolved since the first review:
  - Double-finalize — fixed cleanly: spanError no longer finalizes, and each path now runs finalize exactly once via .ensuring(finalize). Good.
  - Unmatched requests marked as errors — fixed: a Failure now only sets a 400-error span when it contains a real decode Error/InvalidValue, and the test was flipped to assert an unmatched request does not set ERROR.
  - semconv version downgrade — gone.
  - Added the child-span nesting test.

  Still worth addressing:

  1. 4xx is reported as a span ERROR (finalizeSpan: isServerError || isClientError). The reference OpenTelemetryTracing and the OTel HTTP-server semconv set span status ERROR only for 5xx — a 4xx is a client error, not
  a server-span error. Recommend dropping the isClientError arm.
  2. Streaming path is still untested. The spec adds a ZioStreams import but every interpreter is still NoStreams, so the stream branch + handleStreamError (the production path for zio-http) has no coverage. Relatedly,
  the stream branch never sets ERROR status for a streamed 4xx/5xx (only the non-stream branch checks). A streaming test would catch this.
  3. config.spanName is dead config — the initial span name is hardcoded to request.showShort (line 81); the spanName field/default is never read (the sibling module does use it). Either wire it in or drop the field.
  4. Module scope (the biggest structural point, unchanged). The published module still bundles a full OTel SDK-bootstrap framework (ZIOpenTelemetryApp/…AppDefault, Traces/Metrics/Logger, noop/*, OtelEndpoint). Every
  other tapir observability module ships only interceptor + config and dependsOn(serverCore); the bootstrap (exporter type, processor, reader interval, env-var names, the ZIOApp base) is left to the user. Recommend
  keeping interceptor + config here and moving the bootstrap to the examples (or a separate *-bootstrap artifact).
  5. Backend coupling. The interceptor imports ZioStreamHttpResponseBody (+ asInstanceOf[B]), and the build still has a double .dependsOn(...) pulling in zioHttpServer and opentelemetryMetrics. Decoupling from the
  backend (e.g. via BodyListener) and trimming to dependsOn(zio, serverCore % CompileAndTest) would match the other modules.
  6. Minors: the two examples are still .skip (uncompiled/unverified); side-effecting OTel calls use ZIO.succeed (an exception there becomes a defect — ZIO.attempt is safer); a few doc/name nits (file OtelEndpoint.scala
  defines object OtlpEndpoint, Logger.scala defines trait Logging).

  Net: the interceptor is now functionally in good shape — correlation works (proven by the new test), lifecycle is clean, and the false-error cases are mostly fixed. The remaining must-do is small (4xx status + a
  streaming test); the larger ask is structural — slim the module to interceptor + config and decouple it from zio-http.

[cheleb]

I've addressed all the point exempt the #5 TBH no idea what/how to achieved this :-/

[cheleb]

Whaow, now I feel really dumb :p