Wire OTLP Trace Metrics Writers into Metrics Aggregators - #11680
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9be5584c2a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return maps | ||
| } | ||
|
|
||
| def "Datadog-Client-Computed-Stats header set when either stats pipeline is enabled (otlpSpanMetrics=#otlpSpanMetrics, nativeMetrics=#nativeMetrics)"() { |
There was a problem hiding this comment.
Move the new stats-header coverage to JUnit 5
The root AGENTS.md says, "Always use JUnit 5" and "Do not write new Groovy / Spock tests"; this adds a new Spock feature method to DDAgentApiTest.groovy. Please move this coverage to a JUnit 5 test (or migrate the existing file) instead of adding new Groovy/Spock test code.
Useful? React with 👍 / 👎.
eefcc42 to
a00f34b
Compare
9be5584 to
3d2a088
Compare
a00f34b to
484cafa
Compare
3d2a088 to
8cd214f
Compare
|
|
||
| static byte[] buildResourceMessage(Config config, boolean includeDatadogResourceAttributes) { | ||
| static byte[] buildResourceMessage( | ||
| Config config, boolean includeDatadogResourceAttributes, boolean includeStatsComputed) { |
There was a problem hiding this comment.
Instead of continually adding boolean flags which could become fragile (i.e. remembering the order of the flags) we could consider providing a Map<String, String> of additional attributes.
Then you could create a static initializer block under TRACE_RESOURCE_MESSAGE
For example, something like:
Map<String, String> extraAttributes = new HashMap<>();
if (Config.get().isTracesSpanMetricsEnabled()) {
extraAttributes.put(STATS_COMPUTED_KEY, "true");
}
TRACE_RESOURCE_MESSAGE =
buildResourceMessage(Config.get(), false, extraAttributes);
The buildResourceMessage method would then iterate over the extraAttributes entries, building them into the final resource payload.
There was a problem hiding this comment.
Good idea! I replaced the static block w/ a helper function that get's called instead. Let me know if you feel strongly about using the static initializer block instead.
mcculls
left a comment
There was a problem hiding this comment.
Some comments/improvements to be addressed - after that this can be merged into the parent PR for a final review of the overall picture.
There was a problem hiding this comment.
More details
The XOR logic in MetricsAggregatorFactory correctly gates OTLP vs native stats, the DroppingPolicy lambda captures the OTLP flag at construction time to always-drop p0 traces when active, and DDAgentApi's Datadog-Client-Computed-Stats header OR-combines both pipelines — matching the existing Config.get() call pattern already present for isApmTracingEnabled(). No behavioral regressions found across 10+ adversarial scenarios covering gRPC fallback extraction, service-name per-point logic, resource attribute selection, and factory selection XOR. Tests could not be executed due to a Java 25 toolchain requirement (only Java 8–21 available in the sandbox), so the analysis is static.
🤖 Datadog Autotest · Commit 0e5a05f · What is Autotest? · Any feedback? Reach out in #autotest
0e5a05f to
e70afbc
Compare
f89c28a
into
mhlidd/otlp_trace_metrics_setup
init init adding exact sums for ok/error remove host.name changes init tests adding datadog-specific datapoint attribtues more specific logging refactor origin usage in test helpers and add origin specific testS remove irrelevant test following rebase rename histogram class and refactor tests force temporality delta on tracemetrics histogram gating error latencies on empty as well update metricintegrationtest cleanup make AggregateEntry public and move trace metrics files to datadog.trace.core.otlp.metrics wire otlp trace metrics thru OtlpMetricsProtoCollector Merge branch 'master' into mhlidd/otlp_trace_metrics_setup jacoco Add Resource Attributes for OTLP Trace Metrics (#11667) * init * wiring datadog resource attributes into metric writer * cleanup * spotless rename write... w/ emit... Wire OTLP Trace Metrics Writers into Metrics Aggregators (#11680) * init * cleanup * init * adding bugfixes for system-tests * remove unnecessary code * cleanup * cleanup pt 2 * cleanup pt 2 * rebasing updates * renaming config to otel.traces.span.metrics.enabled * updating comments * updating buildResourceMessage to take a map instead of multiple booleans * Gate Additional OTLP Trace Metric Features (#11718) add shutdown for OKhttp client in OtlpStatsMetricWriter Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
What Does This Do
This PR wires the OTLP Trace Metrics logic and writers into the existing Metrics Aggregators used for Client Side Stats.
This involves:
ConflatingMetricsAggregatorwhich uses theOtlpStatsMetricWriterintroduced in BETA: Define OTLP Trace Metrics Histogram and Writers #11656Datadog-Client-Computed-Statsheader when OTLP Trace Metrics are active to prevent dual writing in the AgentConflatingMetricsAggregatorfor testing purposesMotivation
Additional Notes
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]