5201 zio interceptor

[cheleb]

No description provided.

[adamw]

Ah, zio-tracing is not yet published, so this will fail in the build. You'll need to comment out the entire file for now, we'll bring it back after a release of an initial version

[adamw]

Some notes from Claude's review:

  Major issues

  1. OpenTelemetry version downgrade (high risk)

  project/Versions.scala changes openTelemetry from 1.62.0 → 1.61.0. This is a global version used by every otel module (opentelemetry-tracing, otel4s-tracing, metrics, etc.). A downgrade is almost certainly
  unintentional and should be reverted. If 1.62.0 is incompatible with zio-opentelemetry 3.1.15, pin the runtime-telemetry artifact separately rather than rolling back the global version.

  2. Module name / scope mismatch

  The module is in tracing/zio-observability with package sttp.tapir.server.tracing.ziotel, but it spans tracing + metrics + logging. Existing modules separate these (tracing/*, metrics/*). Either:
  - restrict the module to tracing only (matching the directory + package), or
  - move it to a new top-level directory and split into tracing/zio-tracing, metrics/zio-metrics-otel, etc.

  The current placement is misleading and inconsistent with project conventions.

  3. Wrong package declaration in the test

  ZIOtelTracingTest.scala:1 declares package sttp.tapir.server.tracing.otel4s but imports from sttp.tapir.server.tracing.ziotel. Copy-paste error — fix to ziotel.

  4. Shared mutable carrier across requests (likely concurrency bug)

  ZIOtelTracingConfig holds a single IncomingContextCarrier[mutable.Map[String, String]]. ZIOtelTracing.apply passes config.carrier directly into tracing.extractSpanUnsafe(...) for every request. If the carrier is
  backed by a mutable map populated from headers per call, concurrent requests will race on it. The carrier should be constructed per-request from the request's headers, not held in config.

  5. Coupled bootstrap forces tracing + metrics + logging

  ZIOtelBase.otelLayer always combines tracing, logging, metrics, and Java17 runtime telemetry. Users who only want tracing must override otelLayer entirely and re-implement the wiring. Split into smaller composable
  layers (e.g. tracingLayer, metricsLayer, loggingLayer) and let users pick.

  serverOptions similarly hard-codes CORSInterceptor.default and defaultServerLog, which doesn't belong in an observability helper at all.

  6. Naming inconsistencies and typos

  - OltpEndpoint, oltpEnvVar, globalOltpEndpointEnvVar — should be Otlp (OpenTelemetry Protocol).
  - File name ZIOObservabiltyExample.skip — missing 'i' in "Observability".
  - setSpanAttibutes (ZIOtelTracing.scala:837) — missing 'r' in "Attributes".
  - Ovverride (example file comment).
  - Scaladoc in ZIOtelTracing.scala:686-707 refers to ZIOpenTelemetryTracing and Otel4z, but the class is ZIOtelTracing (otel4s pattern was copied without updating the docs).
  - Trait file is ZIOpenTelemetry.scala but most other types use the ZIOtel* prefix. Pick one prefix and use it consistently.

  7. Scala 2 / Scala 3 trait asymmetry

  Scala 3 trait takes (resourceName, oltpEnvVar) as constructor params; Scala 2 version has them as abstract/def with a default for oltpEnvVar only. The test files reflect this (Scala 2 overrides resourceName; Scala 3
  passes as ctor arg). The @param name scaladoc is wrong on both sides. Either unify the API (abstract defs on both) or document the divergence.

  8. Thin test coverage

  Only one test (ZIOtelTracingTest) that asserts getFinishedSpanItems.isEmpty() is false. No assertions on:
  - span name / HTTP_ROUTE / status code attributes
  - error path (5xx + thrown exception)
  - context propagation (W3C traceparent header in → parent span)
  - requestAttributes defaults

  Compare against Otel4sTracingTest, which exercises each of these. At a minimum, mirror those scenarios.

  Minor issues

  - ZIOtelTracing.scala:819-822 — the block
  span
    .updateName(name)
  span.setAllAttributes(attributes)
  - is confusing formatting; the chained .updateName(name) result is dropped and the next line starts a new statement. Just write span.updateName(name); span.setAllAttributes(attributes).
  - ZIOtelTracingConfig.scala:1006 — response.code.code.toLong.asInstanceOf[java.lang.Long] — use java.lang.Long.valueOf(...) instead of asInstanceOf.
  - opentelemetry-runtime-telemetry-java17 is alpha and JVM 17+ only. withRuntimeTelemetry: Boolean = true will silently fail on JDK 11. Default to false, or move it behind an explicit opt-in helper.
  - MeterProvider.stdout and TracerProvider.stdout/fluentbit are defined but unused. Drop them unless tests/examples cover them.
  - Trailing whitespace and blank lines throughout (e.g. ZIOtelBase.scala:497-505, :546-551, :579). Run scalafmt.
  - Example file is acknowledged as needing .skip until zio-tracing is published (adamw's PR comment). Fine — but the inline using dep ... tapir-zio-tracing:1.13.18 reference doesn't match the actual module name
  tapir-zio-observability.
  - TracerProvider.grpc etc. accept envVar: String, globalOltpEnvVar: String as positional String parameters — easy to swap accidentally. A small case class (e.g. OtlpEndpointConfig) would be safer.
  - Scaladoc @param name appears on traits that have no name param. Remove or fix.

Regarding the "observability" / tracing module naming - there is already a zio-metrics module (https://tapir.softwaremill.com/en/latest/server/observability.html#zio-metrics). How would it relate to what's provided here? Is it at all possible to have an only-tracing module? Or maybe we should combine them?

[cheleb]

V2, I've moved the modules in a top level observability folder and uses o11y as package

[adamw]

Thanks! I've got another round of reviews. I'm also a bit confused (just as Claude) whether this is otel4s-based, or zio-opentelemetry based?

  Major issues

  1. OpenTelemetry version downgrade — ✅ Fixed. project/Versions.scala:68 is back to openTelemetry = "1.62.0".

  2. Module name / scope mismatch — ⚠️  Partially addressed. The module moved to observability/otel4z and the package was renamed to sttp.tapir.server.o11y.otel4z. Two issues remain:
  - The name otel4z is actively misleading. It collides visually with otel4s but the module does not use otel4s — it uses zio-opentelemetry (zio.telemetry.opentelemetry.tracing.Tracing). The new doc paragraph at
  doc/server/observability.md:529 explicitly (and incorrectly) says "uses the otel4s library under the hood". Either rename to e.g. tapir-zio-opentelemetry and methods to zioOpentelemetryLogging/Metrics/Tracing, or
  actually use otel4s. The current naming will confuse every reader of the docs and the module list.
  - It still bundles tracing + metrics + logging, but composition via Logging/Metrics/Traces traits is now opt-in, so this is acceptable.

  3. Wrong package declaration in the test — ✅ Fixed. ZIOtelTracingTest.scala:1 is package sttp.tapir.server.o11y.otel4z.

  4. Shared mutable carrier across requests — ❌ Not actually fixed; new functional bug. ZIOpenTelemetryTracing.scala:56 defines def newCarrier() = IncomingContextCarrier.default() and :70 calls it per request. So
  per-request isolation is fine. But the carrier is never populated with request headers, so the propagator extracts from an empty map. Every incoming traceparent/tracestate header is ignored, and there is no parent
  context — context propagation is broken. The carrier has to be initialized from request.headers before being passed to extractSpanUnsafe.

  5. Coupled bootstrap forces tracing + metrics + logging — ✅ Resolved by the Logging/Metrics/Traces traits in Providers.scala. serverOptions is no longer hard-coded in the library; the example wires CORS/serverLog
  itself.

  6. Naming inconsistencies and typos
  - OltpEndpoint → OtlpEndpoint: ✅ fixed.
  - ZIOObservabiltyExample.skip: ✅ renamed to ZIOpenTelemetryExample.skip.
  - setSpanAttibutes (missing r): ❌ still there at ZIOpenTelemetryTracing.scala:100, :179.
  - Scaladoc "Interceptor which traces requests using otel4s" at ZIOpenTelemetryTracing.scala:24: ❌ still says otel4s.
  - Inconsistent prefixes: trait/class are ZIOpenTelemetry*, tests are ZIOtel* (ZIOtelTracingTest, ZIOtelTracingTestApp). Also the base trait is ZIOpentelemetryBase (lowercase p) while everything else is
  ZIOpenTelemetry* — pick one casing.

  7. Scala 2 / Scala 3 trait asymmetry — ❌ Still divergent.
  - Scala 3 (scala-3/.../ZIOpenTelemetry.scala:8): trait ZIOpenTelemetry(val resourceName, val version, val environment).
  - Scala 2 (scala-2/.../ZIOpenTelemetry.scala:8): no constructor; resourceName inherited as abstract def, version/environment overridden to None.
  - @param name Scaladoc is wrong on both files; there is no name parameter.
  - The two test apps (scala-2/scala-3) reflect the divergence (override vs ctor arg). Either make both abstract defs or both constructor params, and update the docs.

  8. Thin test coverage — ⚠️  Marginally better. The single test now asserts span name "GET /person" and attribute count 6, but still has no coverage of:
  - error path (5xx response, thrown exception),
  - W3C traceparent-driven parent linkage (especially given the carrier bug above),
  - specific attribute values (HTTP_REQUEST_METHOD, URL_PATH, HTTP_RESPONSE_STATUS_CODE, HTTP_ROUTE),
  - decode-failure / unmatched-endpoint paths.

  Mirroring Otel4sTracingTest is the bar.

  Minor issues

  - java.lang.Long.valueOf(...) replacing asInstanceOf: ✅ fixed.
  - MeterProvider.stdout / TracerProvider.stdout/fluentbit unused defs: ✅ removed.
  - withRuntimeTelemetry/Java17 alpha module: ✅ defused (commented out in example).
  - Example using dep references tapir-otel4z: ✅ matches actual artifact.
  - Confusing chained formatting at old ZIOtelTracing.scala:819-822 (span.updateName(name) then a new statement on span.setAllAttributes): ❌ still present at ZIOpenTelemetryTracing.scala:161-163.

  New issues introduced or worth surfacing

  1. Carrier never reads request headers (see point 4 above) — the most important bug. Context propagation is non-functional.
  2. Doc claim is wrong: doc/server/observability.md:529 says the module uses otel4s — it does not. Same applies to the module name otel4z/methods otel4z*.
  3. environmentTag (ZIOpenTelemetryBase.scala:68-69) is redundant — Tag[Environment] is already derived; this override adds nothing.
  4. Example file still has dead code (commented m <- ZIO.service[Meter] at line 59), inconsistent indentation, and unbalanced empty lines. Run scalafmt before merging.
  5. ZIOtelTracingTestApp.scala in both scala-2 and scala-3 test sources defines object TestZIOApp extends ZIOApp ... but isn't a ZIOSpec and isn't referenced. Either delete or convert into an actual smoke test.
  6. ZIOpenTelemetryLayer.live always layers OpenTelemetry.zioMetrics when withZIOMetrics = true (default). This implicitly requires the OpenTelemetry.metrics("zio") chain to succeed even when the user mixes in only
  Traces. Worth verifying that a tracing-only app still boots without Metrics.

  Summary

  Several of the previous critical issues are fixed (version downgrade, OltpEndpoint typo, file rename, package declaration, coupled bootstrap, Long.valueOf). The blockers that remain:

  - carrier not populated from headers → context propagation broken (must fix),
  - module name otel4z + docs claiming otel4s (confusing/incorrect — must clarify),
  - setSpanAttibutes typo still on a public-ish method,
  - otel4s scaladoc on ZIOpenTelemetryTracing,
  - Scala 2 vs Scala 3 API divergence undocumented,
  - single thin test.

  Minor: chained-updateName formatting, redundant environmentTag, dead code in example, dangling test app, inconsistent ZIOpenTelemetry* vs ZIOtel* vs ZIOpentelemetryBase casing.

[cheleb]

Thanks, sorry for typos and naming :'(

TBH, I'm not sure with solution to the point (4) Carrier && Shared mutable carrier across requests

Should I extract all headers, from the incoming request ?

Response of my LLM:

Yes, it makes perfect sense to extract the carrier from incoming headers in the ZIOpenTelemetryTracing implementation.

This is the standard approach for distributed tracing context propagation in OpenTelemetry. When a request arrives at a service, the tracing context (trace ID, parent span ID, trace flags, etc.) is typically propagated via HTTP headers (following the W3C Trace Context specification with headers like traceparent and tracestate).

The code correctly:

1. Extracts headers from the incoming ServerRequest
2. Converts them to a MutableMap of (name, value) pairs
3. Wraps this in an IncomingContextCarrier.default which understands how to read trace context from these headers
4. Passes this carrier to tracing.extractSpanUnsafe along with the propagator configuration

This allows the service to:

• Continue an existing trace from the caller service (if trace context headers are present)
• Start a new trace if no context is propagated (root span)
• Properly link spans across service boundaries in a distributed system

Without extracting the carrier from headers, each service would start completely independent traces, breaking the end-to-end trace visibility that distributed tracing provides.

The implementation follows OpenTelemetry best practices and is consistent with how other OpenTelemetry instrumentation libraries handle HTTP context propagation.