Remove more manually listed instrumentation helpers - #12688
Open
sarahchen6 wants to merge 5 commits into
Open
sarahchen6 wants to merge 5 commits into
sarahchen6 wants to merge 5 commits into
Conversation
sarahchen6
added
type: feature
Contributor
Contributor
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
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. |
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
sarahchen6
force-pushed
the
sarahchen6/remove-more-helpers
branch
from
September 29, 2026 19:57
b46f939 to
4f921c4
Compare
sarahchen6
added this pull request to stack #12552
September 29, 2026 19:57
sarahchen6
marked this pull request as ready for review
September 29, 2026 21:15
sarahchen6
requested review from
ValentinZakharov and
bric3
and removed request for
a team
September 29, 2026 21:15
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. |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ValentinZakharov
approved these changes
Sep 29, 2026
gh-worker-dd-mergequeue-cf854d
Bot
requested a review
from a team
as a code owner
September 29, 2026 22:46
gh-worker-dd-mergequeue-cf854d
Bot
requested review from
dromanol and
manuel-alvarez-alvarez
September 29, 2026 22:46
ojproductions
approved these changes
Sep 30, 2026
sarahchen6
added
tag: no release notes
…enabled (#12546) Expose OTel thread/process context without requiring profiling - 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 Defer ddprof context integration construction past premain for AppSec-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. Drop dedicated OTel context exposure config flag, derive purely from 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. review: pre-PR checks - 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 review: remove decisions.md from tracked files 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. review: defer ddprof context construction with a startup delay Mitigates a P1 Autotest finding (PR #12546 discussion): the deferred construction was only moved off the premain thread, not delayed past premain/main startup, so it could still race with an application setting java.nio.file.spi.DefaultFileSystemProvider in main. Schedule it with the same delay magnitude Agent already uses for the analogous OkHttp/JUL startup race. review: defer profiling context engine tag until deferred construction succeeds - DeferredProfilingContextIntegration now exposes whenAvailable(Runnable), queuing callbacks until the real ddprof integration swaps in and never running them if construction fails. Default implementation runs inline, so the synchronous profiling/JFR path is unchanged. - CoreTracer stamps the _dd.profiling.ctx.engine tag through that callback instead of an identity check, so the tag is only ever set once the deferred integration actually becomes available, never unconditionally. - Bumped the deferred construction failure log to info, since it's the only signal that requested context exposure silently didn't happen. - Added useJUnitPlatform() to the springboot smoke test's testRuntimeActivation task, fixing the AppSec-inactive branch of OtelContextExposureSmokeTest never running in CI. review: limit testRuntimeActivation to the OTel exposure spec useJUnitPlatform() made the task discover every Spock spec in the source set, but it runs with AppSec forced inactive, which the other specs (e.g. SpringBootSmokeTest's 403 blocking assertions) don't tolerate. Filter the task to OtelContextExposureSmokeTest only. review: fix thread-safety race in CoreTracer local root span tags - Merge localRootSpanTags/localRootSpanTagsNeedIntercept into a single immutable LocalRootSpanTags holder published through one volatile reference, so a concurrent startSpan() never observes the new tag map paired with the stale intercept flag (introduced by this PR's deferred profiling context construction) revert: remove AppSec runtime-activation trigger for OTel context exposure Confirmed with product that no remote-config/one-click activation path is needed for this feature: CADR relies only on SSI and fleet (startup config), never on AppSec turning on later via remote config. Removes ActiveSubsystems.setAppSecActive/whenAppSecActivated and the associated callback list, Config.isOtelContextExposurePendingAppSecActivation, and Agent.createAppSecActivatedDdprofContextIntegration, plus their tests. The fix for the separate double process-context registration issue (Agent.ddprofContextIntegrationFactory's registerProcessContext param) is unrelated and stays untouched. Merge branch 'master' into otel-context-without-profiling fix: unwrap LocalRootSpanTags in DDTracerAPITest reflection CoreTracer.localRootSpanTags moved from a plain Map to a private LocalRootSpanTags{tags, needsIntercept} wrapper in 6b1c10d, so the test's cast to java.util.Map now throws ClassCastException. Reflect into the wrapper's tags field before casting. docs: remove stale remote-config activation paragraph from DeferredProfilingContextIntegration javadoc The runtime-activation path this described was reverted in b1d3bbb; createDdprofContextIntegration() is now the only caller of scheduleInitialization(). fix: forward virtual-thread context binding in deferred wrapper; tighten 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. fix: exclude AWS Lambda from ddprof context-exposure path; cover factory-level registration failure in smoke test docs: trim verbose Javadocs and inline comments per reviewer feedback Reduce multi-paragraph Javadocs to 1-2 lines across the OTel context exposure changes, keeping only non-obvious rationale. refactor: self-document OTel context config naming per reviewer feedback 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. refactor: split ddprof context integration into load/defer paths per 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. refactor: drop redundant Windows check in createProfilingContextIntegration Config.isDatadogProfilerEnabled() and Config.isOtelThreadContextEnabled() already resolve to false on Windows via isDatadogProfilerSafeAndConfigured (isDatadogProfilerEnablementOverridden() checks OperatingSystem.isWindows()), so the local OperatingSystem.isWindows() check here was a second, independently-maintained exclusion. AWS Lambda has no equivalent Config-side guard, so !isAwsLambdaRuntime() stays. test: cover null-returning factory in DeferredProfilingContextIntegration initialize() already guards against a null integration from the factory, but no test constructed one, so a mutation deleting that guard would have survived (delegate=null, NPE on any pass-through call after initialize()). test: rename disabledInAnEnvironmentWhereTheDatadogProfilerIsUnsafe The method name overclaimed what the test covers: it drives the same raw-predicate short-circuit as the explicit-disable test via an env var, not a genuine environment-detection veto (already explained in the javadoc). Renamed for honesty; no behavior change. Merge branch 'master' into otel-context-without-profiling Add explicit profiling guard and needsIntercept regression test - Make the !isProfilingEnabled() invariant on the ddprof context-only branch explicit in Agent.createProfilingContextIntegration(), per reviewer feedback that there was no local guard against skipping the JFR-events fallback. - Add a regression test pinning the needsIntercept() recomputation in CoreTracer.stampProfilingContextEngine(), covering the trace.split-by-tags scenario raised in review. fix: register OTel process context before constructing ddprof trace-context integration Process context registration for CWS/eBPF must not depend on the trace-context (ddprof) integration succeeding: it already handles its own failures, but the previous ordering meant a throwing newDdprofContextIntegration call (e.g. native library init failure) silently prevented registerProcessContext from ever running. Merge branch 'master' into otel-context-without-profiling fix: skip OtelContextExposureSmokeTest on J9 AbstractSmokeTest forces -Ddd.profiling.ddprof.enabled=false on J9 (known jmethodID crashes), so the OTel process context registration this test asserts on is unreachable by construction there, not flaky. J9 stays covered by ConfigOtelContextExposureTest. Merge branch 'master' into otel-context-without-profiling Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
sarahchen6
force-pushed
the
sarahchen6/remove-more-helpers
branch
from
September 30, 2026 16:49
4f921c4 to
60c4143
Compare
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
robcarlan-datadog
approved these changes
Sep 30, 2026
Contributor
Author
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in Use ⏳ Building merge commit dc650fb0cf in pipeline 8943165997995790055... |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Remove more manually listed instrumentation helpers
Motivation
Follow-up to #12649 that cleans up entire or partial lists of instrumentation helpers
Additional Notes
There are ~157 remaining overrides still present int his codebase, but they are required for Muzzle compilation.
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: [PROJ-IDENT]