feat: add setTraceId and clearTraceId to ProfilerApi#313
Conversation
|
|
e8b271d to
7827187
Compare
korniltsev-grafanista
left a comment
There was a problem hiding this comment.
I would prefer if we did not use the labels infrastructure (ScopedContext, LabelsSet) for this and instead passed trace id through setTracingContext and JFR (that would require async-profiler changes, we can put trace id as well as spanId and spanName into the thread local structure they recently added asprof_thread_local_data ).
The main reason is that the current implementation will break in few ways:
- If one already associated labels to the thread with
ScopedContext, the new set of methods will not be aware of it and silently override it. - Similarly the other way around: if we set labels with
setTraceIdand then someone else usesScopedContext- it will override trace id label, unless we expose the new java ThreadLocal somehow, which may be not trivial, especially across class loaders.
Another reason is performance (premature?) optimizations. I got no proof, but gut feeling tells me storing __int128 would be cheaper then keeping every traceId strings between profile uploads.
| @Override | ||
| public void setTraceId(@NotNull String traceId) { | ||
| // W3C trace ID is 32 hex chars (128 bits). Split into two longs. | ||
| long hi = Long.parseUnsignedLong(traceId.substring(0, 16), 16); |
There was a problem hiding this comment.
String#substring allocates, can we somehow try parsing hexes without it?
There was a problem hiding this comment.
Pull request overview
This PR extends the cross-classloader ProfilerApi with setTraceId(String) / clearTraceId() so the OTel integration can associate a 128-bit W3C trace ID with samples, and updates the embedded async-profiler fork version/distribution artifacts accordingly.
Changes:
- Add
setTraceId(String)andclearTraceId()as default methods onProfilerApifor backward-compatible implementations. - Implement
setTraceId/clearTraceIdinProfilerSdkby parsing a 32-hex-char trace ID into twolongs and delegating to async-profiler. - Bump bundled Grafana async-profiler version and update
.so.sha1checksums; add unit tests for hex parsing.
Reviewed changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Bumps the Grafana async-profiler version used by the download script. |
| async-profiler-grafana-fork-dist/lib/libasyncProfiler-macos.so.sha1 | Updates checksum for the macOS native library. |
| async-profiler-grafana-fork-dist/lib/libasyncProfiler-linux-x64.so.sha1 | Updates checksum for the Linux x64 native library. |
| async-profiler-grafana-fork-dist/lib/libasyncProfiler-linux-arm64.so.sha1 | Updates checksum for the Linux arm64 native library. |
| agent/src/test/java/io/pyroscope/javaagent/ProfilerSdkTest.java | Adds unit tests validating parseHex64 parsing behavior. |
| agent/src/main/java/io/pyroscope/javaagent/ProfilerSdk.java | Implements trace ID support and introduces parseHex64. |
| agent/src/main/java/io/pyroscope/javaagent/api/ProfilerApi.java | Adds the new API methods as defaults for compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Adds two methods to
ProfilerApiso the OTel integration can attach the trace ID as a string label on profile samples.setTracingContext(long, long)cannot carry a 128 bit value, hence a separate entry point.The
ProfilerSdkimpl backs them with aThreadLocal<ScopedContext>that opensScopedContext("trace_id", traceId)on set and closes it on clear, reusing the existing labels pipeline.Both new methods are declared
default(no op) on the interface so older runtimes stay wire compatible with newer callers (the label simply does not appear).Related PR https://github.com/grafana/otel-profiling-java/pull/TBD will land once this is released.