-
Notifications
You must be signed in to change notification settings - Fork 165
feat: support OpenTelemetry Metrics #3487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3487 +/- ##
==========================================
- Coverage 61.73% 61.65% -0.08%
==========================================
Files 143 143
Lines 13015 13015
Branches 1702 1702
==========================================
- Hits 8035 8025 -10
- Misses 4221 4228 +7
- Partials 759 762 +3 see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2025-12-15 21:08:34 Comparing candidate commit 0670208 in PR branch Found 4 performance improvements and 15 performance regressions! Performance is the same for 175 metrics, 0 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache
scenario:ContextPropagationBench/benchExtractHeaders128Bit-opcache
scenario:ContextPropagationBench/benchExtractTraceContext64Bit-opcache
scenario:ContextPropagationBench/benchInject128Bit-opcache
scenario:HookBench/benchWithoutHook-opcache
scenario:LogsInjectionBench/benchLogsNullBaseline-opcache
scenario:LogsInjectionBench/benchLogsNullInjection-opcache
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching2
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SamplingRuleMatchingBench/benchRegexMatching4
scenario:SpanBench/benchOpenTelemetryAPI
scenario:SpanBench/benchOpenTelemetryAPI-opcache
scenario:TraceSerializationBench/benchSerializeTrace
scenario:WordPressBench/benchWordPressBaseline-opcache
scenario:WordPressBench/benchWordPressDdprof-opcache
scenario:WordPressBench/benchWordPressOverhead-opcache
|
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| if (\dd_trace_env_config('DD_TRACE_REPORT_HOSTNAME')) { | ||
| $attributes['host.name'] = \dd_trace_env_config('DD_HOSTNAME'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid overriding host.name with empty DD_HOSTNAME
When DD_TRACE_REPORT_HOSTNAME is true but DD_HOSTNAME is unset, this hook always adds a host.name attribute with an empty string before calling ResourceInfo::merge. In OpenTelemetry, merge gives precedence to the attributes of the resource being merged in, so this empty value will overwrite the hostname the upstream Host detector already populated. The result is that OTel metrics exported with hostname reporting enabled lose the actual hostname and carry a blank host.name. Skip setting the attribute when DD_HOSTNAME is empty or fall back to the detected hostname to avoid clobbering the original value.
Useful? React with 👍 / 👎.
| } else if (params_count == 3 && FUNCTION_NAME_MATCHES("track_telemetry_metrics")) { | ||
| zval *metric_name = ZVAL_VARARG_PARAM(params, 0); | ||
| zval *metric_value = ZVAL_VARARG_PARAM(params, 1); | ||
| zval *tags = ZVAL_VARARG_PARAM(params, 2); | ||
| if (Z_TYPE_P(metric_name) == IS_STRING && Z_TYPE_P(tags) == IS_STRING) { | ||
| ddtrace_metric_register_buffer(Z_STR_P(metric_name), DDOG_METRIC_TYPE_COUNT, DDOG_METRIC_NAMESPACE_TRACERS); | ||
| ddtrace_metric_add_point(Z_STR_P(metric_name), zval_get_double(metric_value), Z_STR_P(tags)); | ||
| RETVAL_TRUE; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Major)
I believe we should type-validate metric_value here, just as we did for metric_name and tags, because currently we are calling zval_get_double(metric_value) without verifying it's numeric.
This may or may not cause a crash if someone passes a non-numeric vakue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bob just told me that "zval_get_double() is infallible and always safe to use", so perhaps I made you do this change for nothing, oops 😬
PROFeNoM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just one or two little things and its good to go for me 👍
| } else if (params_count == 3 && FUNCTION_NAME_MATCHES("track_telemetry_metrics")) { | ||
| zval *metric_name = ZVAL_VARARG_PARAM(params, 0); | ||
| zval *metric_value = ZVAL_VARARG_PARAM(params, 1); | ||
| zval *tags = ZVAL_VARARG_PARAM(params, 2); | ||
| if (Z_TYPE_P(metric_name) == IS_STRING && Z_TYPE_P(tags) == IS_STRING) { | ||
| ddtrace_metric_register_buffer(Z_STR_P(metric_name), DDOG_METRIC_TYPE_COUNT, DDOG_METRIC_NAMESPACE_TRACERS); | ||
| ddtrace_metric_add_point(Z_STR_P(metric_name), zval_get_double(metric_value), Z_STR_P(tags)); | ||
| RETVAL_TRUE; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bob just told me that "zval_get_double() is infallible and always safe to use", so perhaps I made you do this change for nothing, oops 😬
Description
Changes:
DD_METRICS_OTEL_ENABLED(enable OTel metrics support) andDD_HOSTNAMEDD_TRACE_AGENT_URLand/orDD_AGENT_HOSTPlease refer to this document containing known discrepancies of the PHP OTel Metrics implementation
Reviewer checklist