Repository navigation
Conversation
vpellan
added
area:opentelemetry
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-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The startup-log predicate correctly requires semantics mode plus an enabled OpenTelemetry signal, and excludes OTLP trace export when tracing is disabled. The specialist’s completed static review identified no actionable regression.
🤖 Bits Code Review · Commit 7341d63
| [InlineData(ConfigurationKeys.FeatureFlags.OpenTelemetryMetricsEnabled, "true", true, true)] | ||
| [InlineData(ConfigurationKeys.FeatureFlags.OpenTelemetryMetricsEnabled, "true", false, false)] | ||
| #endif | ||
| public void IsOpenTelemetryModeEnabled_RequiresSemanticsModeAndSignal(string signalKey, string signalValue, bool otelSemanticsEnabled, bool expected) |
There was a problem hiding this comment.
Given "is open telemetry mode enabled" seems kinda arbitrary, I'm not sure it makes sense to have all these tests for it 😄 They're just going to immediately break whenever you change what it means, and they're just a bunch of booleans anyway 🤷♂️
There was a problem hiding this comment.
We're creating a system-test to unify what it does: DataDog/system-tests#8158
There was a problem hiding this comment.
Even more reason to get rid of this test then 😉
There was a problem hiding this comment.
Fair, removed them in c045b30. Test_OTEL_Startup_Logs in DataDog/system-tests#8158 covers each case end to end.
| || instanceSettings.OtlpLogsExportEnabled | ||
| || instanceSettings.OtelSemanticsEnabled | ||
| || (mutableSettings.TraceEnabled && exporterSettings.IsOtlpTraceExport)); | ||
| writer.WriteValue(instanceSettings.IsOpenTelemetryModeEnabled(mutableSettings, exporterSettings)); |
There was a problem hiding this comment.
Tbh, I'm not really sure it makes sense to report all this as "OTEL_ENABLED". It probably makes more sense to either split this into purely
- otel_semantics_enabled
- otel_exporting_enabled
or tbh, just remove it entirely - a single boolean trying to summarise so many things probably does more harm than good.
There was a problem hiding this comment.
There will be an environment variable in the future that will do exactly this, to enable Otel mode with a single env var. It's not prioritized because the recommended way of using DDOT sets all the required env var for you, but it will be added at some point. The goal for now is to look with a single value if OTel mode works
There was a problem hiding this comment.
But "Otel mode" means 4 different things here, as indicated by ||. If these were && I could understand it, but right now Otel mode doesn't mean one thing according to this
There was a problem hiding this comment.
The env var will activate all signals,, but if you only care about traces, you can disable OtlpLogsExportEnabled and OtlpMetricsExportEnabled, but OTel mode is still enabled, which is why it's an || here. Then the detail is printed as logs after that. Otel mode means Otel semantics is enabled, Otel exporter is otlp and a signal is enabled. However, looking more into that made me realise that IsActivityListenerEnabled does not belong here, as it's not the signal for OTel trace export
There was a problem hiding this comment.
I mean, this still doesn't make sense to me 🤷♂️ If you're looking at introducing an OTEL_ENABLED mode, then shouldn't this just tell you that setting?
There was a problem hiding this comment.
We haven't agreed yet on the naming convention for that new config, but we want to add an equivalent in the logs before re:invent
There was a problem hiding this comment.
Correction to my earlier answer: IsActivityListenerEnabled (DD_TRACE_OTEL_ENABLED) does belong here, but only together with OTLP trace export. The trace signal needs both the OpenTelemetry tracing API and OTLP trace export (DD_TRACE_OTEL_ENABLED=true and OTEL_TRACES_EXPORTER=otlp). If either is off, the trace signal is off. If metrics and logs are off too, OTEL_ENABLED is false.
So OTel mode is now: semantics mode && (OTLP metrics || OTLP logs || (tracing API && OTLP traces)). DataDog/system-tests#8158 has been updated to match, and the fix is in 3c2bed0.
There was a problem hiding this comment.
I agree with Andrew, I'm not sure I see the value in reporting a catch all OTEL_ENABLED value, though I guess this got introduced in #9376. I understand that we want to set up one environment variable that effectively turns on a whole set of OpenTelemetry environment variables, but I prefer doing a direct reporting of environment variable to value and when we introduce this "meta" environment variable we can also report that environment variable as its own entry.
I feel like we already have every individual setting here, right?
- OTel Traces API:
DD_TRACE_OTEL_ENABLED - OTel Trace OTLP Export:
OTEL_TRACES_EXPORTER - OTel Metrics API & OTLP Export:
DD_METRICS_OTEL_ENABLED - OTel Logs API & OTLP Export:
DD_LOGS_OTEL_ENABLED - OTel semantics:
DD_TRACE_OTEL_SEMANTICS_ENABLED
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9520) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-10-28 18:34:12 Comparing candidate commit b1f6fb9 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 71 metrics, 1 unstable metrics, 5 known flaky benchmarks, 121 flaky benchmarks without significant changes.
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a8106d4 to
881dadf
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
system-tests covers OTEL_ENABLED end to end in Test_OTEL_Startup_Logs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
the
area:tracer
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vpellan
changed the title
Summary of changes
OTEL_ENABLEDin theDATADOG TRACER CONFIGURATIONstartup log is nowtrueonly when at least one OpenTelemetry signal is on:DD_METRICS_OTEL_ENABLED)DD_LOGS_OTEL_ENABLED)DD_TRACE_OTEL_SEMANTICS_ENABLED), the OpenTelemetry tracing API (DD_TRACE_OTEL_ENABLED) and OTLP trace export (OTEL_TRACES_EXPORTER=otlp), when tracing is enabledSemantics mode only affects traces, so it only counts towards the trace signal. None of the three trace settings turns on the trace signal by itself.
Reason for change
APMAPI-2649: the formula added in #9376 ORed every setting, so semantics mode alone,
DD_TRACE_OTEL_ENABLED=truealone, or OTLP trace export alone each reportedtrue.Implementation details
The check moves to
TracerSettings.IsOpenTelemetryModeEnabled(MutableSettings, ExporterSettings), next to the existingOtlpMetricsExportEnabledandOtlpLogsExportEnabledchecks. The startup log passes its current mutable and exporter settings, as before.Test coverage
Test_OTEL_Startup_Logsfrom test(otel): check that OTEL_ENABLED requires semantics mode and a signal system-tests#8158 covers each case end to end, so this PR doesn't add unit tests for the check.Other details
OTEL_TRACES_EXPORTERtootlp. With both PRs, the tracing API with semantics mode turns on the trace signal.OTEL_SDK_DISABLED). Combined with it,OTEL_SDK_DISABLED=trueturns every signal off, soOTEL_ENABLEDreportsfalse.🤖 Generated with Claude Code