profiles: improve JFR export example by jhalliday · Pull Request #8349 · open-telemetry/opentelemetry-java

jhalliday · 2026-04-30T13:18:02Z

Add metadata to the OTLP message so as to make it more interpretable by receiving backends.

codecov · 2026-04-30T13:24:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.76%. Comparing base (824334c) to head (5e09b77).
⚠️ Report is 151 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8349      +/-   ##
============================================
- Coverage     78.77%   78.76%   -0.02%     
  Complexity     8579     8579              
============================================
  Files          1009     1009              
  Lines         28993    28993              
  Branches       3599     3599              
============================================
- Hits          22839    22836       -3     
- Misses         5311     5312       +1     
- Partials        843      845       +2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jhalliday · 2026-04-30T13:38:19Z

@open-telemetry/profiling-maintainers This PR is somewhat interesting, as one of the early examples of an end-to-end interop where the sender and receiver are written by different people working more or less in isolation from one another. It exposes some rough edges that may represent opportunities/requirements for spec changes to make it a smoother process.

Some metadata is required by the backend, but not by the spec. On the one had the backend (devfiler) is behaving reasonably - it can't interpret data and render visualizations without it. On the other hand, unless I missed something it's not specified or required by the spec at present. "thread.name", for example. Opportunity to add 'SHOULD'-level recommendations to the semconv?
The 'null' dict_table[0] element is defined as a wire-level empty value to minimize wire bytes, but in the case of Link the trace spec already defined that a) the span/trace are of fixed, non-zero length and b) specifically the invalid value of each is (in string form) "0000...". These requirements are contradictory. I think the profiling wire spec should go with the trace spec definition, despite the extra bytes. In practice it will compress anyhow and the hassle of retrofitting trace codecs to deal with zero length strings does not appeal. Here for example the existing Java SDK will throw if "" is used and it's hard to call that wrong, so code duplication or config flags are the only available routes to evolve it.

zeitlinger

Nice metadata additions. Two suggestions:

Hoist hot-loop dictionary lookups. In JfrExecutionSampleEventConverter.accept() the "thread.name" key index and KeyValueAndUnitData are rebuilt for every sample event. Same in JfrLocationDataCompositor.frameToLocation() for "profile.frame.type"/"jvm" per frame. The dict dedupes so output is correct, but each call still allocates a string + KeyValueAndUnitData. Since the key/value pair is constant per converter, compute it once (e.g. in the constructor or lazily cached) and reuse the int index.

For the thread sample, only threadName/threadNameData vary — pre-compute the "thread.name" key index once.

Null sampledThread. recordedEvent.getValue("sampledThread") can be null for some ExecutionSample variants. A null guard (skip or fall back to "unknown") would harden the converter against truncated/synthetic events.

LGTM otherwise — the ValueTypeData fix and frame-type attribute look right.

jhalliday · 2026-06-22T13:56:55Z

Hi Gregor

Thanks for taking a look.

Hoist hot-loop dictionary lookups.

Right. There are two subtly different cases here, where one KV is entirely constant and the other is dependent on the event's thread value. That's partly an artifact of an over-simplification I made to assume all frames are "jvm" type. If native code is involved then the value there also becomes event-dependent.

Nevertheless there is still a performance argument for caching, since the number of thread names / thread types is considerably smaller than the number of events, but it's a classic space/time tradeoff to add a HashMap for these and at this early stage I'm lacking data to support it. The thread name case in particularly is concerning, as it's an unbounded key space and thus unbounded cache size. I'm going with 'the cost of churning a short lived key object is tolerable', especially since the frame's nameFrom computation is a worse example of the same issue and will likely dominate either of the others.

Null sampledThread. recordedEvent.getValue("sampledThread") can be null for some ExecutionSample variants.

Can it? The asserts in OpenJDK's jfrThreadSampling.cpp seemed to indicate it's always set, but ok, no real downside to hedging anyhow. I think the main takeaway here is the testing thus far is just a handful of old JFR files I had lying around and they don't contain some obvious alternative cases - the frame name code will break on non-java frames I think, but there aren't any in the test set... The JFR event APIs make it next to impossible to mock JFR data cleanly, which is a colossal pain for testing. OpenJDK itself seems to do it by having a curated collection of (hand crafted?) JFR files in version control instead.

opentelemetry-pr-dashboard · 2026-07-18T13:39:12Z

Pull request dashboard status

Waiting on the author · refreshed 2026-08-21 20:19 UTC

Respond to 5 review items (e.g. link a commit, explain why not, ask a follow-up):

Inline threads: 1, 2, 3, 4, 5

Status above doesn't look right?

Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

jack-berg

Hey sorry it took me so long to get involved in this.

I have a better system now for cutting through the endless noise of github. If you see I miss something like this, please ping me. 🙂

jack-berg · 2026-08-21T20:18:05Z

-    linkTable.putIfAbsent(LinkData.create("", ""));
+    // TODO this is, strictly speaking, probably not profile spec compliant at present.
+    // The spec uses "" but the Id encoders don't like that. The alpha spec may need revision...
+    linkTable.putIfAbsent(LinkData.create(TraceId.getInvalid(), SpanId.getInvalid()));


PR title advertises an example-only change, but this modifies the shared SDK dictionary null-element semantics. Worth calling out in the PR description.

jack-berg · 2026-08-21T20:18:05Z

-    linkTable.putIfAbsent(LinkData.create("", ""));
+    // TODO this is, strictly speaking, probably not profile spec compliant at present.
+    // The spec uses "" but the Id encoders don't like that. The alpha spec may need revision...
+    linkTable.putIfAbsent(LinkData.create(TraceId.getInvalid(), SpanId.getInvalid()));


Nothing in this PR appears to reference linkTable[0]. What specifically breaks with "", and where? Ideally the fix belongs in the id encoder (accept "" as invalid) rather than inflating the placeholder here.

jack-berg · 2026-08-21T20:18:05Z

+    // TODO this is, strictly speaking, probably not profile spec compliant at present.
+    // The spec uses "" but the Id encoders don't like that. The alpha spec may need revision...


Is there an upstream spec issue tracking this? If so, link it from the TODO; if not, worth filing one so the divergence has a paper trail and can be reverted once resolved.

jack-berg · 2026-08-21T20:18:05Z

+    }
+
+    String threadName = recordedThread.getJavaName();
+    int threadNameIndex = profilesDictionaryCompositor.putIfAbsent("thread.name");


Narrowing zeitlinger's suggestion: even without a per-thread cache, the "thread.name" key index itself is constant. Compute once in the constructor and store as an int field. No unbounded cache concern, and it drops one HashMap lookup + string allocation per event.

jack-berg · 2026-08-21T20:18:05Z

+
    LocationData locationData =
-        LocationData.create(0, 0, List.of(lineData), Collections.emptyList());
+        LocationData.create(0, 0, List.of(lineData), List.of(typeAttributeIndex));


If the frame-type KV is hoisted to a field, hoist List.of(typeAttributeIndex) alongside it. Same applies to List.of(attribIndex) in JfrExecutionSampleEventConverter once the attribute index is stable per thread.

jhalliday requested a review from a team as a code owner April 30, 2026 13:18

jhalliday mentioned this pull request May 1, 2026

profiles: improve Link message encoding documentation open-telemetry/opentelemetry-proto#792

Merged

This was referenced May 4, 2026

Pull Request Dashboard #8366

Closed

Pull Request Dashboard #8375

Closed

zeitlinger reviewed May 20, 2026

View reviewed changes

This was referenced May 26, 2026

Pull Request Dashboard #8425

Closed

Pull Request Dashboard #8439

Open

jhalliday mentioned this pull request Jun 11, 2026

profiles: handling null pointers into dictionary tables open-telemetry/opentelemetry-proto#812

Open

thswlsqls mentioned this pull request Jun 22, 2026

Fix Javadoc errors in JFR profiles shim #8503

Merged

jhalliday added 2 commits June 22, 2026 13:13

profiles: improve JFR export example

d42ad0a

profiles: improve JFR export example

5e09b77

jhalliday force-pushed the jh-profiling-t branch from 9032cb5 to 5e09b77 Compare June 22, 2026 13:54

jack-berg reviewed Aug 21, 2026

View reviewed changes

Navigation Menu

Uh oh!

profiles: improve JFR export example - #8349

profiles: improve JFR export example#8349
jhalliday wants to merge 2 commits into
open-telemetry:mainfrom
jhalliday:jh-profiling-t

jhalliday commented Apr 30, 2026

Uh oh!

codecov Bot commented Apr 30, 2026 •

edited

Loading

Uh oh!

jhalliday commented Apr 30, 2026

Uh oh!

zeitlinger left a comment

Uh oh!

jhalliday commented Jun 22, 2026

Uh oh!

opentelemetry-pr-dashboard Bot commented Jul 18, 2026 •

edited

Loading

Uh oh!

jack-berg left a comment

Uh oh!

jack-berg Aug 21, 2026

Uh oh!

jack-berg Aug 21, 2026

Uh oh!

jack-berg Aug 21, 2026

Uh oh!

jack-berg Aug 21, 2026

Uh oh!

jack-berg Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// TODO this is, strictly speaking, probably not profile spec compliant at present.
		// The spec uses "" but the Id encoders don't like that. The alpha spec may need revision...

Uh oh!

Conversation

jhalliday commented Apr 30, 2026

Uh oh!

codecov Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jhalliday commented Apr 30, 2026

Uh oh!

zeitlinger left a comment

Choose a reason for hiding this comment

Uh oh!

jhalliday commented Jun 22, 2026

Uh oh!

opentelemetry-pr-dashboard Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request dashboard status

Uh oh!

jack-berg left a comment

Choose a reason for hiding this comment

Uh oh!

jack-berg Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

jack-berg Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

jack-berg Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

jack-berg Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

jack-berg Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 30, 2026 •

edited

Loading

opentelemetry-pr-dashboard Bot commented Jul 18, 2026 •

edited

Loading