Skip to content

profiles: add JFR data export example#8207

Draft
jhalliday wants to merge 3 commits intoopen-telemetry:mainfrom
jhalliday:jh-profiling-r
Draft

profiles: add JFR data export example#8207
jhalliday wants to merge 3 commits intoopen-telemetry:mainfrom
jhalliday:jh-profiling-r

Conversation

@jhalliday
Copy link
Copy Markdown
Contributor

Sample code for converting and exporting JFR ExecutionSample events using OTLP profiles signal.

@jhalliday jhalliday requested a review from a team as a code owner March 23, 2026 15:39
@jhalliday jhalliday marked this pull request as draft March 23, 2026 15:40
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.25%. Comparing base (a6fadff) to head (67bb561).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8207      +/-   ##
============================================
- Coverage     90.27%   90.25%   -0.02%     
- Complexity     7684     7687       +3     
============================================
  Files           850      850              
  Lines         23186    23198      +12     
  Branches       2352     2354       +2     
============================================
+ Hits          20931    20938       +7     
- Misses         1530     1532       +2     
- Partials        725      728       +3     

☔ View full report in Codecov by Sentry.
📢 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.

*
* <p>This class is not threadsafe and must be externally synchronized.
*/
public class JfrLocationDataCompositor {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exporter module should only have exporter functions in it. Eventually, when OTLP profiles stabilize, this will be merged into opentelemetry-exporter-otlp, at which point it won't be acceptable to have JFR artifacts.

This JFR stuff seems best described as a shim or bridge, like https://github.com/open-telemetry/opentelemetry-java/tree/main/opencensus-shim or https://github.com/open-telemetry/opentelemetry-java/tree/main/opentracing-shim.

The question is whether it made sense for it to be hosted in this repo or elsewhere - i.e. opentelemetry-java-instrumentation or opentelemetry-java-contrib. And whether there is anything in common across profile instrumentations (i.e. shims / bridges) that should be extracted to a common place, like a profile API / SDK module.

I haven't seen ProfilesDictionaryCompositor before just now. It looks like it was added while I was OOO. This seems to be the "anything in common across profile instrumentations" bit, and is what allows this JFR piece to be so small / compact.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think you're right conceptually, but the situation is muddied a little by a couple of factors.

JFR is part of the JDK. Unlike the other bridges, it's not brining in additional dependencies. Also, profiling doesn't have a separate API module, so moving the JFR pieces elsewhere still leaves them close coupled to the not-really-public interface of the exporter.

As you noticed, as much as possible of the code doesn't use JFR APIs, so the JFR layer is quite thin. However, it's using naming and design patterns that are the same as in the exporter utility code and for easy of use should be kept in sync with them. If the JFR layer moves elsewhere, I'd be tempted to pull out the bits that are not JFR specific and yet also not part of the normal exporter pattern used by other signal's exporters and move those too. That's probably the DictionaryCompositor and SampleCompostion bits in addition to the JFR package.

I think my preferred option would be to keep the JFR and non-JFR not-exporter pieces a) together and b) in this repo, but in a separate module from the exporter. Splitting them up makes it harder to keep design consistency between them and moving them to a different repo makes it harder to keep them in sync with the exporter. However, both those problems should lessen over time as things stabilise, so maybe the short (relatively!) term pain is worth it to get a better long term structure, which would point more towards moving bits into e.g. contrib.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JFR is part of the JDK. Unlike the other bridges, it's not brining in additional dependencies. Also, profiling doesn't have a separate API module,

Yeah we don't have any precedent for this type of thing to lean on. To reiterate one thing I've said previously, logs started out with a design similar to this, with a goal of only having an SDK for bridging support where the bridges would directly depend on the SDK. It turned out to be non practical and so we separated out a thin log API.

One practical thing we could do:

  • opentelemetry-sdk-profiles: Home of ProfileExporter and ease of use utilities like ProfilesDictionaryCompositor. Its reasonable to want to have alternative profile exporters besides OTLP. Minimally things like an OTLP logging exporter, but also maybe someone would want to build a pprof exporter. Having a dedicated profile SDK module allows you us to build such things without them needing to depend on OTLP.
  • opentelemetry-jfr-profile-shim: Home of the JFR translation bits. Since there's not a lot to this, I wouldn't be heartbroken if this was merged into opentelemetry-sdk-profiles, but I do think that some logical separate between shared bits and implementation specific bits is useful. I agree with you that its useful to keep the JFR bits close to the ProfilesDictionaryCompositor so they can be developed in tandem, but someday after things stabilize, it may make more sense to move JFR out to a dedicated module in opentelemetry-java-instrumentation. In this case, it will benefit from having been split out.

Thoughts @open-telemetry/java-approvers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-berg ok, take a look at this and see if it's along the lines you intended. If so I'll continue and factor out the JFR parts too. That will have to happen - they need the exporter, but that would create a circular dependency if they stay where they are.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! Left a few comments.

*/

package io.opentelemetry.exporter.otlp.profiles;
package io.opentelemetry.sdk.profiles;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convention for the classes/interfaces representing the in-memory data model is to put them in io.opentelemetry.sdk.{signal}.data:

See metrics for a reference: https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data

}

description = "OpenTelemetry - Profiles SDK"
otelJava.moduleName.set("io.opentelemetry.sdkprofiles")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
otelJava.moduleName.set("io.opentelemetry.sdkprofiles")
otelJava.moduleName.set("io.opentelemetry.sdk.profiles")

*/
@Immutable
@AutoValue
public abstract class ImmutableFunctionData implements FunctionData {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you get away with putting these in the root io.opentelemetry.sdk.profiles package and keeping the classes and create methods package private? Always better to avoid more public internal API surface area if reasonable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not unless I put the JFR code there too. By extension of that argument, anything wanting to avoid having to reimplement the interfaces would have to be in the same package. You're favouring lower maintenance burden over utility to the user and implicitly arguing that e.g. ImmutableSpanContext being public was a design error?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're favouring lower maintenance burden over utility to the user and implicitly arguing that e.g. ImmutableSpanContext being public was a design error?

Yes given that ImmutableSpanContext is internal and the create method can be accessed alternatively via the public SpanContext.create, there doesn't seem to be any value in ImmutableSpanContext being public

This is all experimental and so it doesn't need to be perfect out of the gate. Benefit of getting it right first try is reduced churn, but some is inevitable so its not a big deal. In my head, the composition classes are the public facing utilities that allow you to incrementally build up the profiles data model, and the autovalue implementations are an internal implementation detail of the composition classes. I do see that JFR uses a few of the internal immutable data classes directly. Didn't look close enough to figure out why, but:

  1. We're moving away from shared internal code Strengthen versioning requirements #6970, so I think if it will be common for clients to need to instantiate instances of the data model, best to follow the lead of SpanContext and add a dedicated public create method on the data model interface itself
  2. If its not super common to create instances of the data interfaces but still sometimes happens, its easy enough for the dependent code to create their own autovalue implementations

Comment thread settings.gradle.kts
include(":integration-tests:graal-incubating")
include(":javadoc-crawler")
include(":opencensus-shim")
include(":opentelemetry-sdk-profiles")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
include(":opentelemetry-sdk-profiles")
include(":sdk:profiles")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the intent on that one, its matching the fs module layout currently. Do you want /opentelemetry-sdk-profiles moved to /sdk/profiles on the filesystem too? I'd assumed not and stuck it in its own top level thing at least until it's stable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want /opentelemetry-sdk-profiles moved to /sdk/profiles on the filesystem too?

Yes. Stability is governed separately by including a grade.properties with otel.release=alpha in the module root. For example: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/incubator/gradle.properties

* of a JFR recording file. This is not a supported CLI and is not intended to be configurable by
* e.g. command line flags.
*/
public class JfrExportTool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is just a demo, should move somewhere else, like the test module. Not important at the moment since this isn't published yet. A TODO in the build.gradle.kts file above the commented // id("otel.publish-conventions") line should suffice for now.

// and therefore a bit of a pain to get gradle to compile against...
compileJava {
sourceCompatibility = "1.8"
targetCompatibility = "1.8"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: I need to take a closer look at this. Why does it work? Is there a more idiomatic way to achieve?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that was from Lauri in #7741 (comment)
When the JFR bits get moved into their own module it will go with them, or we can just drop it and declare the JFR functionality as available only on 9+ which probably won't upset too many people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants