Add tracing for colibri requests - #633
Conversation
|
Hi, thanks for your contribution! |
7c13727 to
54dd341
Compare
WIP: the context is not being read correctly for some reason
54dd341 to
6f75d31
Compare
aaronkvanmeerten
left a comment
There was a problem hiding this comment.
Thanks for this — the overall approach (remote parent from the IQ extension, mirroring the jicofo side) looks right to me. A few observations beyond the inline comments:
The WIP "context is not being read correctly" issue is most likely already fixed. JibriIqProvider used to drop all child extensions when parsing, so the TraceParent never made it onto the parsed JibriIq. That was fixed in jitsi/jitsi-xmpp-extensions#147 ("Fix Jibri IQ extension parsing"), and the version this PR pins (1.0-118-g64ecd07) is exactly that commit — the WIP commit predates the bump. Worth an end-to-end retest and squashing the WIP note out of the history.
Spans will be silently dropped at process exit — worst in single-use mode. TracingGlobal uses a BatchSpanProcessor (default 5s export delay) and never flushes; jibri exits via exitProcess() in cleanupAndExit (Main.kt). With jibri.single-use-mode = true the process exits right after the session ends — exactly when the most valuable spans were just created, so the jibri.stop span (and any session spans) will routinely be lost. Ideally jicoco-tracing would expose a shutdown()/forceFlush() (the sdk field is typed as the OpenTelemetry interface, so callers can't reach the SdkTracerProvider without a cast today); short of that, a JVM shutdown hook in Main that casts to OpenTelemetrySdk and closes it works.
On the jicofo side (jitsi/jicofo#1301): in the pending-timeout stop path, the TraceParent is added to startIq instead of stopRequest (copy-paste). Also both the timeout-stop and retry paths run on timer threads where Context.current() is root, so those stops will never carry a trace context — only user-initiated stops handled inside the jibri.request span will. Worth flagging over there.
Design note on the wire format (non-blocking): the extension carries trace_id/parent_id/trace_flags as separate attributes and drops tracestate entirely (TraceState.getDefault() on the receive side). Carrying the full W3C traceparent string (plus optional tracestate) in the extension and reusing W3CTraceContextPropagator with a trivial TextMapGetter over the IQ would eliminate the hand-rolled parse/build on both ends and stay spec-aligned. Might be worth settling before the format ossifies across jicofo/jibri/jvb. Relatedly, TracingGlobal builds the SDK with no propagators configured, which any future HTTP-header-based propagation (e.g. the ktor instrumentation on jibri's HTTP API) would silently trip over — adding W3CTraceContextPropagator there would future-proof it.
I'm putting together a follow-up branch with prototypes for the suggestions here (session-lifetime span with state-machine events, context propagation into the async service start, flush-on-exit, OTLP config defaults, and traceparent extraction on the HTTP API) and will link it from this PR.
|
As promised in my review, I've pushed a branch with prototypes for the suggestions: tracing-jibri...tracing-jibri-prototypes It's based on this PR's head, with one commit per suggestion so they can be cherry-picked individually:
Compile, ktlint, and the full test suite pass on the branch. Feel free to cherry-pick whatever's useful into this PR. |
|
jenkins, add to whitelist |
This PR adds tracing for the handling of colibri requests.
Depends on jitsi/jicofo#1301.
To test this, the following configuration is required
tracing { enabled = true otlp-protocol = "grpc" otlp-endpoint = "http://localhost:4317" }Related to the tracing backend GSoC project: https://summerofcode.withgoogle.com/programs/2026/projects/GseggmSv