Repository navigation
Conversation
jbachorik
changed the title
c98c2a0 to
52c579e
Compare
|
This pull request has been marked as stale because it has not had activity over the past quarter. It will be closed in 7 days if no further activity occurs. Feel free to reopen the PR if you are still working on it. |
52c579e to
340ae63
Compare
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 12 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.058 s) : 0, 1057846
Total [baseline] (11.007 s) : 0, 11007201
Agent [candidate] (1.066 s) : 0, 1065583
Total [candidate] (11.189 s) : 0, 11188886
section appsec
Agent [baseline] (1.26 s) : 0, 1259761
Total [baseline] (11.001 s) : 0, 11001103
Agent [candidate] (1.265 s) : 0, 1265377
Total [candidate] (11.008 s) : 0, 11008154
section iast
Agent [baseline] (1.235 s) : 0, 1234875
Total [baseline] (11.328 s) : 0, 11328182
Agent [candidate] (1.234 s) : 0, 1234219
Total [candidate] (11.341 s) : 0, 11340962
section profiling
Agent [baseline] (1.189 s) : 0, 1189232
Total [baseline] (11.082 s) : 0, 11082069
Agent [candidate] (1.188 s) : 0, 1188392
Total [candidate] (11.112 s) : 0, 11112255
gantt
title petclinic - break down per module: candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.245 ms) : 0, 1245
crashtracking [candidate] (1.237 ms) : 0, 1237
BytebuddyAgent [baseline] (632.627 ms) : 0, 632627
BytebuddyAgent [candidate] (637.115 ms) : 0, 637115
AgentMeter [baseline] (29.523 ms) : 0, 29523
AgentMeter [candidate] (29.812 ms) : 0, 29812
GlobalTracer [baseline] (249.021 ms) : 0, 249021
GlobalTracer [candidate] (250.429 ms) : 0, 250429
AppSec [baseline] (32.297 ms) : 0, 32297
AppSec [candidate] (32.625 ms) : 0, 32625
Debugger [baseline] (59.875 ms) : 0, 59875
Debugger [candidate] (60.195 ms) : 0, 60195
Remote Config [baseline] (625.989 µs) : 0, 626
Remote Config [candidate] (595.217 µs) : 0, 595
Telemetry [baseline] (8.012 ms) : 0, 8012
Telemetry [candidate] (8.823 ms) : 0, 8823
Flare Poller [baseline] (8.387 ms) : 0, 8387
Flare Poller [candidate] (8.424 ms) : 0, 8424
section appsec
crashtracking [baseline] (1.256 ms) : 0, 1256
crashtracking [candidate] (1.233 ms) : 0, 1233
BytebuddyAgent [baseline] (672.82 ms) : 0, 672820
BytebuddyAgent [candidate] (676.515 ms) : 0, 676515
AgentMeter [baseline] (12.138 ms) : 0, 12138
AgentMeter [candidate] (12.17 ms) : 0, 12170
GlobalTracer [baseline] (248.548 ms) : 0, 248548
GlobalTracer [candidate] (249.384 ms) : 0, 249384
AppSec [baseline] (186.48 ms) : 0, 186480
AppSec [candidate] (186.61 ms) : 0, 186610
Debugger [baseline] (65.891 ms) : 0, 65891
Debugger [candidate] (66.598 ms) : 0, 66598
Remote Config [baseline] (573.663 µs) : 0, 574
Remote Config [candidate] (579.897 µs) : 0, 580
Telemetry [baseline] (7.86 ms) : 0, 7860
Telemetry [candidate] (7.962 ms) : 0, 7962
Flare Poller [baseline] (3.493 ms) : 0, 3493
Flare Poller [candidate] (3.482 ms) : 0, 3482
IAST [baseline] (24.267 ms) : 0, 24267
IAST [candidate] (24.329 ms) : 0, 24329
section iast
crashtracking [baseline] (1.23 ms) : 0, 1230
crashtracking [candidate] (1.215 ms) : 0, 1215
BytebuddyAgent [baseline] (810.22 ms) : 0, 810220
BytebuddyAgent [candidate] (810.38 ms) : 0, 810380
AgentMeter [baseline] (11.435 ms) : 0, 11435
AgentMeter [candidate] (11.437 ms) : 0, 11437
GlobalTracer [baseline] (239.944 ms) : 0, 239944
GlobalTracer [candidate] (240.13 ms) : 0, 240130
AppSec [baseline] (27.933 ms) : 0, 27933
AppSec [candidate] (28.481 ms) : 0, 28481
Debugger [baseline] (65.932 ms) : 0, 65932
Debugger [candidate] (64.829 ms) : 0, 64829
Remote Config [baseline] (546.729 µs) : 0, 547
Remote Config [candidate] (532.54 µs) : 0, 533
Telemetry [baseline] (7.888 ms) : 0, 7888
Telemetry [candidate] (7.803 ms) : 0, 7803
Flare Poller [baseline] (3.449 ms) : 0, 3449
Flare Poller [candidate] (3.399 ms) : 0, 3399
IAST [baseline] (30.107 ms) : 0, 30107
IAST [candidate] (29.874 ms) : 0, 29874
section profiling
ProfilingAgent [baseline] (94.328 ms) : 0, 94328
ProfilingAgent [candidate] (94.543 ms) : 0, 94543
crashtracking [baseline] (1.195 ms) : 0, 1195
crashtracking [candidate] (1.186 ms) : 0, 1186
BytebuddyAgent [baseline] (694.146 ms) : 0, 694146
BytebuddyAgent [candidate] (693.801 ms) : 0, 693801
AgentMeter [baseline] (9.219 ms) : 0, 9219
AgentMeter [candidate] (9.246 ms) : 0, 9246
GlobalTracer [baseline] (207.463 ms) : 0, 207463
GlobalTracer [candidate] (207.346 ms) : 0, 207346
AppSec [baseline] (33.09 ms) : 0, 33090
AppSec [candidate] (32.946 ms) : 0, 32946
Debugger [baseline] (66.288 ms) : 0, 66288
Debugger [candidate] (65.923 ms) : 0, 65923
Remote Config [baseline] (591.996 µs) : 0, 592
Remote Config [candidate] (577.193 µs) : 0, 577
Telemetry [baseline] (7.83 ms) : 0, 7830
Telemetry [candidate] (7.804 ms) : 0, 7804
Flare Poller [baseline] (3.517 ms) : 0, 3517
Flare Poller [candidate] (3.595 ms) : 0, 3595
Profiling [baseline] (94.889 ms) : 0, 94889
Profiling [candidate] (95.091 ms) : 0, 95091
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.055 s) : 0, 1054596
Total [baseline] (8.882 s) : 0, 8882385
Agent [candidate] (1.061 s) : 0, 1061090
Total [candidate] (8.87 s) : 0, 8870455
section iast
Agent [baseline] (1.243 s) : 0, 1242502
Total [baseline] (9.634 s) : 0, 9634118
Agent [candidate] (1.23 s) : 0, 1229987
Total [candidate] (9.576 s) : 0, 9576132
gantt
title insecure-bank - break down per module: candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.227 ms) : 0, 1227
crashtracking [candidate] (1.236 ms) : 0, 1236
BytebuddyAgent [baseline] (631.427 ms) : 0, 631427
BytebuddyAgent [candidate] (635.978 ms) : 0, 635978
AgentMeter [baseline] (29.497 ms) : 0, 29497
AgentMeter [candidate] (29.91 ms) : 0, 29910
GlobalTracer [baseline] (247.81 ms) : 0, 247810
GlobalTracer [candidate] (250.618 ms) : 0, 250618
AppSec [baseline] (32.257 ms) : 0, 32257
AppSec [candidate] (32.498 ms) : 0, 32498
Debugger [baseline] (58.659 ms) : 0, 58659
Debugger [candidate] (59.361 ms) : 0, 59361
Remote Config [baseline] (593.41 µs) : 0, 593
Remote Config [candidate] (607.426 µs) : 0, 607
Telemetry [baseline] (7.979 ms) : 0, 7979
Telemetry [candidate] (8.791 ms) : 0, 8791
Flare Poller [baseline] (8.963 ms) : 0, 8963
Flare Poller [candidate] (5.903 ms) : 0, 5903
section iast
crashtracking [baseline] (1.247 ms) : 0, 1247
crashtracking [candidate] (1.223 ms) : 0, 1223
BytebuddyAgent [baseline] (817.434 ms) : 0, 817434
BytebuddyAgent [candidate] (808.554 ms) : 0, 808554
AgentMeter [baseline] (11.551 ms) : 0, 11551
AgentMeter [candidate] (11.412 ms) : 0, 11412
GlobalTracer [baseline] (240.613 ms) : 0, 240613
GlobalTracer [candidate] (239.001 ms) : 0, 239001
IAST [baseline] (29.365 ms) : 0, 29365
IAST [candidate] (29.184 ms) : 0, 29184
AppSec [baseline] (29.748 ms) : 0, 29748
AppSec [candidate] (26.756 ms) : 0, 26756
Debugger [baseline] (64.452 ms) : 0, 64452
Debugger [candidate] (64.676 ms) : 0, 64676
Remote Config [baseline] (528.078 µs) : 0, 528
Remote Config [candidate] (539.326 µs) : 0, 539
Telemetry [baseline] (7.781 ms) : 0, 7781
Telemetry [candidate] (7.743 ms) : 0, 7743
Flare Poller [baseline] (3.434 ms) : 0, 3434
Flare Poller [candidate] (3.353 ms) : 0, 3353
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 3 performance regressions! Performance is the same for 15 metrics, 18 unstable metrics.
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section baseline
no_agent (1.25 ms) : 1237, 1263
. : milestone, 1250,
iast (3.319 ms) : 3272, 3366
. : milestone, 3319,
iast_FULL (5.999 ms) : 5938, 6059
. : milestone, 5999,
iast_GLOBAL (3.665 ms) : 3602, 3727
. : milestone, 3665,
profiling (2.1 ms) : 2081, 2120
. : milestone, 2100,
tracing (1.961 ms) : 1944, 1978
. : milestone, 1961,
section candidate
no_agent (1.261 ms) : 1249, 1272
. : milestone, 1261,
iast (3.411 ms) : 3365, 3457
. : milestone, 3411,
iast_FULL (5.965 ms) : 5906, 6024
. : milestone, 5965,
iast_GLOBAL (3.669 ms) : 3615, 3722
. : milestone, 3669,
profiling (357.437 µs) : 350, 365
. : milestone, 357,
tracing (1.974 ms) : 1957, 1991
. : milestone, 1974,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section baseline
no_agent (18.229 ms) : 18044, 18414
. : milestone, 18229,
appsec (18.781 ms) : 18593, 18969
. : milestone, 18781,
code_origins (17.752 ms) : 17575, 17929
. : milestone, 17752,
iast (18.05 ms) : 17871, 18229
. : milestone, 18050,
profiling (19.227 ms) : 19037, 19418
. : milestone, 19227,
tracing (17.669 ms) : 17494, 17843
. : milestone, 17669,
section candidate
no_agent (18.958 ms) : 18769, 19147
. : milestone, 18958,
appsec (18.49 ms) : 18310, 18671
. : milestone, 18490,
code_origins (18.707 ms) : 18523, 18890
. : milestone, 18707,
iast (17.856 ms) : 17684, 18028
. : milestone, 17856,
profiling (255.456 µs) : 245, 266
. : milestone, 255,
tracing (17.603 ms) : 17434, 17773
. : milestone, 17603,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section baseline
no_agent (1.491 ms) : 1479, 1503
. : milestone, 1491,
appsec (3.844 ms) : 3622, 4066
. : milestone, 3844,
iast (2.286 ms) : 2216, 2357
. : milestone, 2286,
iast_GLOBAL (2.321 ms) : 2251, 2392
. : milestone, 2321,
profiling (2.101 ms) : 2046, 2156
. : milestone, 2101,
tracing (2.097 ms) : 2043, 2151
. : milestone, 2097,
section candidate
no_agent (1.489 ms) : 1478, 1501
. : milestone, 1489,
appsec (3.851 ms) : 3625, 4077
. : milestone, 3851,
iast (2.282 ms) : 2212, 2352
. : milestone, 2282,
iast_GLOBAL (2.331 ms) : 2260, 2401
. : milestone, 2331,
profiling (2.109 ms) : 2053, 2164
. : milestone, 2109,
tracing (2.1 ms) : 2046, 2154
. : milestone, 2100,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.62.0-SNAPSHOT~2ac3d50b80, baseline=1.62.0-SNAPSHOT~c72f06780f
dateFormat X
axisFormat %s
section baseline
no_agent (15.618 s) : 15618000, 15618000
. : milestone, 15618000,
appsec (14.91 s) : 14910000, 14910000
. : milestone, 14910000,
iast (18.599 s) : 18599000, 18599000
. : milestone, 18599000,
iast_GLOBAL (18.106 s) : 18106000, 18106000
. : milestone, 18106000,
profiling (14.75 s) : 14750000, 14750000
. : milestone, 14750000,
tracing (14.973 s) : 14973000, 14973000
. : milestone, 14973000,
section candidate
no_agent (15.067 s) : 15067000, 15067000
. : milestone, 15067000,
appsec (14.752 s) : 14752000, 14752000
. : milestone, 14752000,
iast (18.773 s) : 18773000, 18773000
. : milestone, 18773000,
iast_GLOBAL (18.118 s) : 18118000, 18118000
. : milestone, 18118000,
profiling (14.772 s) : 14772000, 14772000
. : milestone, 14772000,
tracing (15.189 s) : 15189000, 15189000
. : milestone, 15189000,
|
jbachorik
added
comp: profiling
|
This pull request has been marked as stale because it has not had activity over the past quarter. It will be closed in 7 days if no further activity occurs. Feel free to reopen the PR if you are still working on it. |
2ac3d50 to
f46659b
Compare
8a2f5e1 to
c664462
Compare
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
d06c3a7 to
c17270a
Compare
jbachorik
changed the title
c15cff2 to
e8e27f3
Compare
a5583d0 to
c047c12
Compare
…et, exclude .claude/state
6e59d5b to
c2332f4
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 207f9f5a6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Provisional — the OTLP profiles proto is still in v1development; update when stabilized | ||
| public static final String DEFAULT_OTLP_HTTP_PROFILES_ENDPOINT = "v1/profiles"; | ||
| public static final String DEFAULT_OTLP_GRPC_PROFILES_ENDPOINT = | ||
| "opentelemetry.proto.collector.profiles.v1.ProfilesService/Export"; |
There was a problem hiding this comment.
When OTLP profiles uses the default endpoint—or gRPC appends this service path—the request targets the nonexistent stable v1 profiles API even though this change encodes the provisional v1development schema. Current collectors expose /v1development/profiles and opentelemetry.proto.collector.profiles.v1development.ProfilesService/Export; the validation test masks the mismatch by accepting HTTP 404, so default HTTP and gRPC exports can fail without testing detecting it.
Useful? React with 👍 / 👎.
| long jfrSize = Files.size(jfrFile); | ||
| try (InputStream jfrStream = Files.newInputStream(jfrFile)) { | ||
| encoder.writeBytesField(OtlpProtoFields.Profile.ORIGINAL_PAYLOAD, jfrStream, jfrSize); |
There was a problem hiding this comment.
perf: In the default LIGHT mode, a large JFR is not actually streamed with flat heap usage: writeBytesField copies all jfrSize bytes into the growing ProtobufEncoder buffer, and toByteArray() then creates another full-size copy. The resulting array remains retained during upload, with up to ten concurrent sends allowed, so sufficiently large or overlapping recordings can consume several times their combined size and OOM the instrumented application; apply a size cap or make the request body stream directly instead.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
| parser.handle(ExecutionSample.class, this::handleExecutionSample); | ||
| parser.handle(MethodSample.class, this::handleMethodSample); | ||
| parser.handle(ObjectSample.class, this::handleObjectSample); |
There was a problem hiding this comment.
In CONVERTED mode on Windows, unsupported ddprof platforms, or configurations with ddprof disabled, CompositeController supplies only the OpenJDK profiler, whose template records jdk.ExecutionSample and newer jdk.CPUTimeSample events. These handlers register only the datadog.* CPU, wall, and allocation interfaces, so those environments silently export profiles without their primary CPU samples; register the corresponding JDK event types or reject converted mode when no supported sample source is present.
Useful? React with 👍 / 👎.
| @Override | ||
| public void release() { | ||
| // noop | ||
| protected void doRelease() { | ||
| // heap-backed; nothing to free | ||
| } |
There was a problem hiding this comment.
On Oracle JDK 8 with OTLP export enabled, the OTLP listener consumes and closes the first JfrRecordingStream, whose close() immediately calls helper.closeRecording(recordingId); the classic uploader then runs as the second listener and can no longer open its stream. The new reference count cannot protect this resource while doRelease() remains a no-op, so closing the recording should move to doRelease() and individual streams should close only their stream handles.
Useful? React with 👍 / 👎.
| recordingData.retain(); | ||
| retainedRecordings.add(recordingData); |
There was a problem hiding this comment.
If a stream-backed recording throws while getStream() or addStream() copies it, addRecording() propagates the exception after already retaining and storing the recording. A caller that abandons the failed converter and releases its own reference therefore leaves the backing recording permanently retained, and a partially copied temporary file remains until JVM exit; remove and release the entry, and delete any temporary file, when registration fails.
Useful? React with 👍 / 👎.
| attributes.put("telemetry.sdk.name", "datadog"); | ||
| attributes.put("telemetry.sdk.version", TRACER_VERSION); | ||
| attributes.put("telemetry.sdk.language", "java"); | ||
| return Collections.unmodifiableMap(attributes); |
There was a problem hiding this comment.
When users configure DD_TAGS or other profiling tags, this builder emits only the fixed service, environment, version, host, and SDK attributes. The classic uploader includes Config.getMergedProfilingTags(), and the existing OTLP resource builder includes Config.getGlobalTags(), so profiles sent through this path lose attributes used for filtering and grouping and no longer match the application's traces; merge the configured tags with the same canonical-key filtering before returning.
Useful? React with 👍 / 👎.
| // the recording's own time range is unknown before parsing, so convert the full window — | ||
| // a narrow [now-60s, now] fallback would silently drop every sample from an older recording | ||
| Instant end = Instant.now(); | ||
| Instant start = Instant.EPOCH; | ||
|
|
||
| for (Path input : inputPaths) { | ||
| System.out.println(" Adding: " + input); | ||
| converter.addFile(input, start, end); |
There was a problem hiding this comment.
Every standalone CLI conversion assigns Instant.EPOCH through the current time as the profile window, and addFile() uses these values directly for time_unix_nano and duration_nano rather than filtering events. Consequently an ordinary recording is reported as a decades-long profile—and an imported recording with future timestamps can have samples outside its declared window—which can distort or invalidate downstream time-based profile processing; derive the bounds from the JFR chunks/events instead.
Useful? React with 👍 / 👎.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
|
|
||
| private void parseJfrEvents(Path jfrFile) throws IOException { | ||
| try (TypedJafarParser parser = TypedJafarParser.open(jfrFile, parsingContext)) { | ||
| parser.handle(ExecutionSample.class, this::handleExecutionSample); |
There was a problem hiding this comment.
When ddprof is unavailable or disabled, OpenJDK recordings use standard JDK sampling events such as jdk.ExecutionSample and jdk.NativeMethodSample, but the converter registers only datadog.* event handlers. Converted and full exports therefore silently lose the recording's CPU samples in these environments.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
| // stream) instead of emitting a mismatched payload. | ||
| long jfrSize = Files.size(jfrFile); | ||
| try (InputStream jfrStream = Files.newInputStream(jfrFile)) { | ||
| encoder.writeBytesField(OtlpProtoFields.Profile.ORIGINAL_PAYLOAD, jfrStream, jfrSize); |
There was a problem hiding this comment.
In the default LIGHT mode, the encoder reads the entire JFR into nested protobuf buffers and then copies the completed message again. Large recordings can consume several times their size on the application heap and cause an out-of-memory failure instead of providing the documented flat-memory streaming behavior.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
| encoder.writeBytesField(OtlpProtoFields.Profile.PROFILE_ID, profileId); | ||
|
|
||
| // Fields 9 & 10: original_payload_format and original_payload (if enabled) | ||
| if (includeOriginalPayload && !pathEntries.isEmpty()) { |
There was a problem hiding this comment.
In FULL mode, encodeProfile embeds the complete JFR separately in every non-empty CPU, wall, allocation, and lock profile. A recording containing all four sample kinds can therefore expand the allowed 64 MiB original payload to roughly 256 MiB before additional protobuf copies, risking agent heap exhaustion.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
| // the recording's own time range is unknown before parsing, so convert the full window — | ||
| // a narrow [now-60s, now] fallback would silently drop every sample from an older recording | ||
| Instant end = Instant.now(); | ||
| Instant start = Instant.EPOCH; |
There was a problem hiding this comment.
Every CLI conversion labels the profile as starting at the Unix epoch and ending at conversion time. Because these bounds are profile metadata rather than parser filters, historical recordings are exported with a decades-long, incorrect duration instead of their actual recording interval.
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
| recordingData.retain(); | ||
| retainedRecordings.add(recordingData); | ||
| Path file = recordingData.getPath(); | ||
| if (file != null) { | ||
| return addFile(file, recordingData.getStart(), recordingData.getEnd()); | ||
| } | ||
| try (InputStream stream = recordingData.getStream()) { | ||
| return addStream(stream, recordingData.getStart(), recordingData.getEnd()); | ||
| } |
There was a problem hiding this comment.
If path lookup, stream acquisition, or stream copying fails after retain(), addRecording exits without releasing the added reference. Once the caller releases its reference, the backing recording remains retained and cannot clean up unless the failed converter is later reset.
| recordingData.retain(); | |
| retainedRecordings.add(recordingData); | |
| Path file = recordingData.getPath(); | |
| if (file != null) { | |
| return addFile(file, recordingData.getStart(), recordingData.getEnd()); | |
| } | |
| try (InputStream stream = recordingData.getStream()) { | |
| return addStream(stream, recordingData.getStart(), recordingData.getEnd()); | |
| } | |
| recordingData.retain(); | |
| try { | |
| retainedRecordings.add(recordingData); | |
| Path file = recordingData.getPath(); | |
| if (file != null) { | |
| return addFile(file, recordingData.getStart(), recordingData.getEnd()); | |
| } | |
| try (InputStream stream = recordingData.getStream()) { | |
| return addStream(stream, recordingData.getStart(), recordingData.getEnd()); | |
| } | |
| } catch (IOException | RuntimeException | Error e) { | |
| retainedRecordings.remove(recordingData); | |
| recordingData.release(); | |
| throw e; | |
| } |
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
| log.warn("OTLP profile upload rejected: too many concurrent requests"); | ||
| OtlpTelemetry.getInstance().onProfilesExportComplete(false); |
There was a problem hiding this comment.
When all sender threads are occupied, a rejected asynchronous upload records an export failure but never records an attempt because sendAndRelease is not invoked. Under backpressure, telemetry can consequently report more failures than attempts and produce invalid export-rate metrics.
| log.warn("OTLP profile upload rejected: too many concurrent requests"); | |
| OtlpTelemetry.getInstance().onProfilesExportComplete(false); | |
| log.warn("OTLP profile upload rejected: too many concurrent requests"); | |
| OtlpTelemetry.getInstance().onProfilesExportAttempt(); | |
| OtlpTelemetry.getInstance().onProfilesExportComplete(false); |
Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session
|
🗿 🤖 🔴 This is the verdict/bundle summary of the Bits review, not an independent finding: it names no file, line, or symbol beyond the two themes already covered by items 4134449271/4134541667 (JDK sampling loss) and 4134449264/4134541682/4134541696 (LIGHT/FULL heap amplification), all of which are verdicts needs_fix in this evaluation. The summary itself requires no code change; fixing it separately would double-count the inline items. |
mhlidd
left a comment
mhlidd
left a comment
There was a problem hiding this comment.
FWIW some of the shared OTLP mechanisms should probably be refactored by the Capabilities team.
Some design questions about Config handling and Resource Attributes
| @@ -159,15 +159,19 @@ | |||
| "version": "A", | |||
| "type": "decimal", | |||
| "default": "0.5", | |||
| "aliases": ["DD_API_SECURITY_DOWNSTREAM_REQUEST_ANALYSIS_SAMPLE_RATE"] | |||
There was a problem hiding this comment.
nit: was this formatting change intentional? This file alone blows up the PR size 😅
| "aliases": [] | ||
| } | ||
| ], | ||
| "DD_OTLP_PROFILES_HEADERS": [ |
There was a problem hiding this comment.
We marked DD_OTLP_TRACES_HEADER as sensitive so the value doesn't leak into telemetry. Should we do the same here?
| otlpTracesEndpoint = otlpTracesEndpointFromEnvironment; | ||
|
|
||
| // OTLP Profiles config — mirrors the traces/logs/metrics pattern | ||
| int profilesTimeout = |
There was a problem hiding this comment.
Are there DD equivalents of these configs? If so, should we allow users to also set the native DD config as a backup option? That's what we've done for all other OTel configs that we've introduced - see OtelEnvironmentConfigSource.java (ref).
| configProvider.getEnum( | ||
| OTLP_PROFILES_COMPRESSION, OtlpConfig.Compression.class, OtlpConfig.Compression.NONE); | ||
|
|
||
| String otlpProfilesEndpointFromEnvironment = configProvider.getString(OTLP_PROFILES_ENDPOINT); |
There was a problem hiding this comment.
Do we want to fallback to generic OTEL_EXPORTER_OTLP_* configs here? That's a trend that we have for traces, metrics, and logs, but can be optional here if you feel like it's not correct. https://github.com/DataDog/dd-trace-java/blob/master/utils/config-utils/src/main/java/datadog/trace/bootstrap/config/provider/OtelEnvironmentConfigSource.java#L341-L343
| } | ||
|
|
||
| // mirrors the tracer's OTLP traces export resource attributes (OtlpResourceAttributes) | ||
| private static Map<String, String> buildResourceAttributes(Config config) { |
There was a problem hiding this comment.
Can we reuse OtlpResourceAttributes here? Why do we need to replicate the code? (ref)
| long conversionStartNanos = System.nanoTime(); | ||
| OtlpPayload payload = convertToOtlp(data); | ||
| long conversionNanos = System.nanoTime() - conversionStartNanos; | ||
| OtlpTelemetry.getInstance().onProfilesConversion(conversionNanos); |
There was a problem hiding this comment.
From Claude:
Skewed conversion telemetry. onProfilesConversion runs before the null check. It is also recorded in LIGHT mode, which is the default and converts nothing. Result: otel.profiles_conversion_ms and otel.profiles_conversion_max_ms mostly measure file-stat or encode time. Record only in CONVERTED/FULL mode and only when a payload was produced. Also confirm that all five new tracers metrics are in the backend telemetry metric registry: the two gauges plus otel.profiles_export_{attempts,successes,failures}. Serializing doubles as gauges is fine.
| { | ||
| "version": "A", | ||
| "type": "string", | ||
| "default": "light", |
There was a problem hiding this comment.
| "default": "light", | |
| "default": "LIGHT", |
nit: enum values in this file are uppercased.
This branch was previously deployed
2 participants
What Does This Do
Adds OTLP profile export to the agent: a JFR-to-OTLP/P converter plus an uploader wired into the profiling subsystem, sharing the same OTLP transport the tracer already uses for traces/logs/metrics. Off by default.
profiling-otel— hand-coded protobuf encoder with dictionary-based compression (string/function/location/stack/attribute/link tables), no external dependencies. Supportsdatadog.ExecutionSample(CPU),datadog.MethodSample(wall clock),datadog.ObjectSample(allocation),jdk.JavaMonitorEnter/jdk.JavaMonitorWait(lock contention). Samples are emitted one-per-JFR-event (stacks, functions, locations and attributes are deduplicated via the dictionary tables);period/period_typeare emitted only for the periodic CPU/wall profiles, omitted for the event-driven alloc/lock profiles; optional original-payload embedding; PROTO and JSON encodings produce identical content.convert-jfr.shCLI wrapper; validated via profcheck and protoc under TestContainers; JMH benchmarks included.OtlpSender/OtlpHttpSender/OtlpGrpcSenderand the request-body/payload helpers fromdd-trace-coreinto a sharedcommunication:otlp-exportermodule; bundled once in the agent jar'sshared/section. OnlyHTTP_PROTOBUFandGRPCprotocols are supported; anything else fails fast.OtlpProfileUploaderwires the converter and sender intoProfilingAgent. The scrubber wraps the OTLP listener, so OTLP exports scrubbed recordings. A sender-construction failure degrades OTLP export to disabled instead of aborting the profiling subsystem. Embedded original payloads are capped at 64 MiB — oversized recordings are exported converted-only. Temp recordings are deleted on every exit path.RecordingDataimplements reference counting (base count 1, implemented in the OpenJDK/Oracle/ddprof controllers):retain()after full release throws, over-release is a silent no-op,doRelease()fires exactly once when the count reaches zero.profiling.otlp.enabled(defaultfalse),profiling.otlp.mode=light|converted|full(defaultlight).lightships the raw JFR as the original-payload blob without conversion,convertedemits converted samples only,fullemits both.LightweightOtlpEncodercovers the raw-blob path, streaming the payload from disk with the size re-stat'ed immediately before the stream opens. RegisteredDD_OTLP_PROFILES_{ENDPOINT,HEADERS,PROTOCOL,COMPRESSION,TIMEOUT}andDD_PROFILING_OTLP_{ENABLED,MODE}insupported-configurations.json, mirroring the traces/logs/metrics pattern.communication/otlp-exporter/→apm-sdk-capabilities-java,docker/Dockerfile.profcheck→profiling-java.jafar-parserandjmc-flightrecorder-writerdependencies) and the agent jar size budget to 35,200,000 bytes (~33.6 MiB).TempLocationManagerTestconcurrent cleanup deadlines from 30s to 60s.Motivation
Lets the agent export profiles in the OpenTelemetry profiles format over the same OTLP transport the tracer already uses for traces/logs/metrics.
Additional Notes
Upstream maturity: not for production use
The OTLP profiles signal is not stable upstream, so this export must not be used in production:
profiles/*andcollector/profiles/*are at Development maturity for both binary protobuf and JSON (opentelemetry-proto v1.11.1 maturity table).When OTLP profiles export is enabled, the uploader logs a startup warning that the protocol is at Development maturity and must not be used in production. The warning is also sent to instrumentation telemetry.
Performance
JDK 21, macOS Darwin 25.4.0:
ByteArrayOutputStream.writewent from 13.7% to 0% after switching to a directbyte[]+ cursor.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issue— the 7 new keys (
DD_OTLP_PROFILES_{ENDPOINT,HEADERS,PROTOCOL,COMPRESSION,TIMEOUT},DD_PROFILING_OTLP_ENABLED,DD_PROFILING_OTLP_MODE) are registered in the Feature Parity Dashboard, which feeds the public config registry the Java config page renders from (see PROF-15986)Jira ticket: PROF-15986
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.