Conversation
- Add Config.isDatadogProfilerSafeAndConfigured() as the raw ddprof env-safety/explicit-flag predicate, without the isProfilingEnabled() AND-prefix - Add Config.isOtelContextExposureEnabled(), defaulting to enabled when the profiler is safe/configured and either profiling is enabled or AppSec is fully enabled, with an explicit DD_TRACE_OTEL_CONTEXT_EXPOSURE_ENABLED override - Gate Agent.createProfilingContextIntegration()'s ddprof branch on the new flag (additive, ORed with the existing profiling gate) and reflectively register the process context even when profiling never starts - Add TRACE_OTEL_CONTEXT_EXPOSURE_ENABLED to OtlpConfig and supported-configurations.json
jandro996
added
type: feature
|
@codex review |
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. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 2940f6aa6e
ℹ️ 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".
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. |
…-only trigger Constructing DatadogProfilingIntegration touches java.nio.file (via TempLocationManager) and loads the ddprof native library, which must not happen on the primordial premain thread. Users with the Datadog profiler enabled were unaffected (they already ran this synchronously), but the new AppSec-only trigger reached this construction from premain for the first time. DeferredProfilingContextIntegration wraps the real integration behind a NoOp delegate until AgentTaskScheduler runs the deferred construction off the premain thread, then swaps it in. The profiler-enabled path keeps the exact synchronous behavior it had before, since profiling accuracy needs every scope from the first one. Addresses a P1 finding from the Codex review on this PR.
…profiling/AppSec isOtelContextExposureEnabled() no longer has its own explicit override. It mirrors isProfilingEnabled(), which has no dedicated sub-flag either: the kill switch is disabling DD_PROFILING_ENABLED and DD_APPSEC_ENABLED, the same flags that already drive the derivation. This removes the DD_TRACE_OTEL_CONTEXT_EXPOSURE_ENABLED public config entirely, along with its metadata/supported-configurations.json entry - so there is no new config requiring Feature Parity Dashboard registration, which was causing the config-inversion-local-validation.py CI job to fail.
- Add OtelContextExposureSmokeTest verifying OTel process context registration follows AppSec activation, not profiling - Extend DeferredProfilingContextIntegrationTest to cover all delegate pass-through methods (onAttach/onDetach/encodeOperationName/encodeResourceName/onRootSpanFinished), not just newScopeState/name
decisions.md is a session-local planning artifact and should not ship as part of the PR diff; it is now added to the global gitignore alongside progress.md/task_plan.md.
jandro996
added
the
tag: override groovy enforcement
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060ff1f2d6
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Looked into the three "maybe worse" startup scenarios:
Given Note: If reviewers agree with this change before merging, I'll open a PR against |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92339b3acd
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…ten smoke test assertion DeferredProfilingContextIntegration forwarded most methods to its delegate but fell back to the interface defaults for isThreadContextBindingRequired() and setContext(Context), so virtual-thread context rebinding silently never happened on the AppSec-only path even after the deferred ddprof integration was swapped in. OtelContextExposureSmokeTest only asserted the 'Registering process context...' log line, which is emitted unconditionally before the actual registration outcome is known. Also assert the failure log line is absent.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33352ec55c
ℹ️ 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".
…ory-level registration failure in smoke test
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
General feedback: this PR adds very verbose comments to files where the code style has been to have succinct and focused comments. Could you do a round of cleanup before merging to try and reduce them to one or two lines. i.e. limit comments to key details which are not obvious from the code |
Reduce multi-paragraph Javadocs to 1-2 lines across the OTel context exposure changes, keeping only non-obvious rationale.
I agree, I’ve already changed it. A few months ago I started adding more comments (also verbose ones) based on a recommendation to provide more context to the AI, but I haven’t noticed much of a difference. |
Rename isOtelContextExposureEnabled() to isOtelThreadContextEnabled() and the internal isDatadogProfilerEnabled field to isDatadogProfilerSafeAndConfigured so each name matches what it actually represents. Also replace remaining em dashes introduced by the previous comment-trimming commit.
mcculls
left a comment
mcculls
left a comment
There was a problem hiding this comment.
A few readability and maintainability comments - approving for the SDK side of things
Also noting for myself that a more modular startup process would have made this integration easier
…reviewer feedback Replace the single createDdprofContextIntegration/ddprofContextIntegrationFactory pair, which reused one boolean flag for two unrelated purposes (defer-vs-sync construction, and process-context registration), with four dedicated methods: loadDdprofContextIntegration, deferDdprofContextIntegration, and the shared private helpers newDdprofContextIntegration and registerProcessContext. Also apply double-checked locking to DeferredProfilingContextIntegration#whenAvailable, now safe since delegate is volatile, and clarify the initialize() Javadoc contract.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
| static ProfilingContextIntegration createProfilingContextIntegration() { | ||
| Config config = Config.get(); | ||
| // AWS Lambda has no ddprof native library support, same as startProfilingAgent(). | ||
| if (!OperatingSystem.isWindows() && !isAwsLambdaRuntime()) { |
There was a problem hiding this comment.
The !OperatingSystem.isWindows() half of this guard looks redundant: Config already zeroes both isDatadogProfilerEnabled() and isOtelThreadContextEnabled() on Windows via isDatadogProfilerSafeAndConfigured, which isDatadogProfilerEnablementOverridden() forces false with its own OperatingSystem.isWindows() check (Config.java around line 4238). Not wrong today, but it's a second, independently-maintained Windows exclusion — if the Config-side check is ever relaxed without noticing this duplicate, someone reasoning from Config alone would conclude the feature now works on Windows while this stale check silently keeps it off.
The !isAwsLambdaRuntime() half is not redundant — Config has no Lambda exclusion — so only the Windows half would be safe to drop.
🤖 Generated with Claude Code
| * #INITIALIZATION_DELAY_MILLIS}. | ||
| */ | ||
| void scheduleInitialization() { | ||
| AgentTaskScheduler.get().schedule(this::initialize, INITIALIZATION_DELAY_MILLIS, MILLISECONDS); |
There was a problem hiding this comment.
[Comment from Claude] INITIALIZATION_DELAY_MILLIS (1s) is a fixed-time heuristic for "main() has had a chance to install its own java.nio.file.spi.DefaultFileSystemProvider," not an actual lifecycle signal — it narrows the premain/java.nio.file race rather than closing it.
An application with slow classloading/DI startup (a large Spring context, or a container with throttled CPU) that installs a custom FileSystemProvider more than 1s into main() would still race with the deferred ddprof native-library load exactly as before this PR, just less often. The PR description already acknowledges this as accepted risk ("not a guarantee"), so this is mostly a request for a one-line confirmation that a fixed delay is intended to be acceptable long-term, rather than a hidden gap.
🤖 Generated with Claude Code
This branch has not been deployed
5 participants
What Does This Do
Config.isDatadogProfilerSafeAndConfigured(), exposing the existing raw Datadog-profilerenv-safety/explicit-flag predicate (native-image/J9/JDK8-aarch64 exclusions included) without the
isProfilingEnabled()AND-prefix thatisDatadogProfilerEnabled()applies.Config.isOtelContextExposureEnabled(): enabled whenever the profiler is safe and configuredand either profiling is enabled or AppSec's activation level is
ProductActivation.FULLY_ENABLED.There is no dedicated override flag - the same way
isProfilingEnabled()has no per-featureoverride beyond
DD_PROFILING_ENABLED, disabling profiling and AppSec (DD_PROFILING_ENABLED,DD_APPSEC_ENABLED) is already the kill switch for this feature too.Agent.createProfilingContextIntegration()'s ddprof branch to gate onisDatadogProfilerEnabled() || isOtelContextExposureEnabled()(additive - never disables ddproffor a user where real profiling already enabled it) instead of solely on
isDatadogProfilerEnabled().DeferredProfilingContextIntegration, which wraps the realDatadogProfilingIntegrationbehind a no-op delegate until
AgentTaskSchedulerruns the construction off the premain thread,then swaps it in. Constructing the real integration touches
java.nio.file(viaTempLocationManager) and loads the ddprof native library, which must not happen on theprimordial premain thread. Profiling-enabled users are unaffected - they keep the exact
synchronous construction they had before, since profiling accuracy needs every scope from the
first one. The new AppSec-only trigger is the first path that could have reached this
construction from premain, so it goes through the deferred wrapper instead.
ProcessContext.register(ConfigProvider)from the same branch, so theprocess-wide OTel context descriptor is published even when
ProfilingAgent.run()never executes(the AppSec-only, profiling-disabled case).
initializeAllContext()is idempotent by design(verified from the ddprof sources), so no double-invocation guard was added.
Why AppSec-only users need this
Today, exposing the OTel thread-local span context and the process-wide descriptor to external
eBPF/CWS consumers is entirely collapsed into
Config.isProfilingEnabled(). AppSec-onlydeployments (profiling disabled, AppSec
FULLY_ENABLED) never get either signal, even thoughneither actually depends on the profiling recording engine running - only on the ddprof native
library being loaded. This change decouples both gates from profiling so an AppSec-only JVM can
be read by the eBPF/CWS consumer, matching cross-tracer precedent (dd-trace-py's decoupled flag,
PHP's plain
DD_APPSEC_ENABLED=truetrigger).Additional Notes
_dd.profiling.ctxwill now appear on AppSec-only users' spans with no real profile behind it -this is an accepted, pre-existing side effect of instantiating
DatadogProfilingIntegration, nota new bug introduced by this change.
DD_TRACE_OTEL_CONTEXT_EXPOSURE_ENABLEDas an explicit override, but it was dropped in favor ofpure derivation from the existing
DD_PROFILING_ENABLED/DD_APPSEC_ENABLEDflags, following thesame "no dedicated sub-flag" precedent
isProfilingEnabled()itself sets.THREAD_CONTEXT_SHARINGsystem-tests scenario (eBPF/CWS end-to-end validation) requires aLinux host with system-probe support and cannot be run from this darwin dev machine - it's a
manual/CI follow-up, not covered by this PR's test suite.
DeferredProfilingContextIntegration's 1-second delay is a mitigation for the premain/java.nio.filerace, not a guarantee - there's no JVM hook for "the application has entered
main". It's still anet improvement: the pre-existing synchronous path for profiling-enabled users has zero mitigation
for that same race today. A class-load-triggered signal was prototyped as a stronger alternative and
rejected - it fires before
main()runs, which is earlier than the delay, so it removes the headstart the delay gives fast-starting apps instead of closing the race.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-70088
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.