feat: publish Linux OTel contexts - #4077
Conversation
|
Benchmarks [ tracer ]Benchmark execution time: 2026-08-06 04:13:57 Comparing candidate commit 995a137 in PR branch Found 8 performance improvements and 5 performance regressions! Performance is the same for 180 metrics, 1 unstable metrics.
|
Benchmarks [ appsec ]Benchmark execution time: 2026-07-30 13:31:53 Comparing candidate commit 6b3710c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.
|
Publish standard OTel process and thread contexts from the tracer and make the profiler consume them for runtime identity and effective service metadata. Handle span, stack, Fiber, configuration, and fork lifecycle changes while retaining the legacy non-Linux path.
This reverts commit 6b3710c.
c7032dc to
29841a6
Compare
Benchmarks [ profiler ]Benchmark execution time: 2026-08-06 03:08:00 Comparing candidate commit 995a137 in PR branch Found 2 performance improvements and 4 performance regressions! Performance is the same for 23 metrics, 7 unstable metrics.
|
![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: 8d01de4429
ℹ️ 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".
| ThreadContextRead::Inactive => ( | ||
| unsafe { libc::pthread_self() as i64 }, | ||
| 0, | ||
| 0, | ||
| ProcessIdentity::default(), | ||
| ), |
There was a problem hiding this comment.
On Linux, when the profiler is loaded with a ddtrace build that still exports ddtrace_get_profiling_context but does not export the new otel_thread_ctx_v1 symbol—for example, during an independent or rolling upgrade of these separately loadable extensions—this inactive branch emits zero span IDs instead of using the legacy API that startup still locates. All profiler samples then lose trace correlation even though a compatible correlation API is available; use the legacy context when the TLS symbol or record is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The tracer and profiler are meant to be upgraded in tandem. They are not designed to be upgraded piecemeal and in the future this will be impossible when the extensions are merged.
Service, environment, version, and configured hostname can vary by request. Omit them from the process-wide context while retaining their thread-context key mapping and active values.
morrisonlevi
changed the title
# Conflicts: # libdatadog
| #ifdef __linux__ | ||
| if (object->ce == ddtrace_ce_root_span_data) { | ||
| ddtrace_detach_otel_thread_context_for_root(ROOTSPANDATA(object)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I need to read more code first, but this definitely is not the correct place for this. This code is called for any freed RootSpanData, regardless of whether it's active (in fact when it's reaching this code, it's should not be active anymore).
There was a problem hiding this comment.
I think this can just be outright removed. Existing code handles this already.
| if (UNEXPECTED(root->otel_context_attributes_generation != DDTRACE_G(otel_context_attributes_generation))) { | ||
| ddtrace_otel_refresh_attribute_values(root); | ||
| } |
There was a problem hiding this comment.
This is the sole place otel_context_attributes_generation gets consumed.
But why? Why do we need to call ddtrace_otel_refresh_attribute_values() in this case?
There was a problem hiding this comment.
The generation check is necessary. It is a lazy invalidation mechanism for the OTel records embedded in multiple root spans.
Each span stack/root owns a separate cached Thread Context record. Nested/manual stacks and suspended fibers can therefore hold records that are not currently attached. Service, environment, or version changes
can affect multiple records because non-entrypoint roots inherit metadata from the entrypoint root.
On a metadata change:
- The active record is refreshed.
- The global generation is incremented.
- Other records remain stale while inactive.
- When one is attached again, the generation mismatch causes it to refresh.
Removing the refresh from ddtrace_otel_attach_stack() would expose stale metadata after switching stacks. This is covered by tests/ext/otel_thread_context_stack_switch.phpt, particularly switching back to
mainRoot after updating its metadata while otherRoot is active.
Probably should leave a code comment here then.
There was a problem hiding this comment.
But then it should only increment then when the actual entrypoint root span is updated, and not any root span in ddtrace_otel_update_attribute_values.
There was a problem hiding this comment.
Also, should we attach simply the actual entrypoint root span to root_span_data as pointer? And compare the local generation vs the generation on the entrypoint root span? Instead of having a thread global generation counter, which invalidates much more than it needs to.
| // Address of otel_thread_ctx_v1 for this root's thread, resolved at root creation to avoid a dynamic TLS lookup | ||
| // on every stack switch. | ||
| void **otel_context_slot; |
There was a problem hiding this comment.
Is this actually that expensive? Any DDTRACE_G() lookup will basically do the same, I hope the __thread dispatch table is in CPU cache at all times.
I would just remove this.
There was a problem hiding this comment.
I actually benchmarked this before making the commit. I had the AI regenerate assembly to show the difference:
The optimization was introduced by e864a7b. I reproduced the relevant assembly using a default-visible global-dynamic TLS symbol in an LLD-linked shared library.
AMD64
Direct TLS assignment:
attach_direct:
push %rbx
mov %rdi, %rbx
lea otel_thread_ctx_v1@TLSGD(%rip), %rdi
call __tls_get_addr@plt
mov %rbx, (%rax)
pop %rbx
retCached slot assignment:
attach_cached:
mov %rsi, (%rdi)
retARM64
Direct TLS assignment:
attach_direct:
stp x29, x30, [sp, #-16]!
mov x29, sp
mov x8, x0
adrp x0, <TLS descriptor page>
ldr x1, [x0, <resolver offset>]
add x0, x0, <descriptor offset>
blr x1
mrs x9, tpidr_el0
str x8, [x9, x0]
ldp x29, x30, [sp], #16
retCached slot assignment:
attach_cached:
str x1, [x0]
retThere was a problem hiding this comment.
On "regular" code this will not matter at all, but it might for fiber heavy code.
|
|
||
| zend_string *service = NULL, *env = NULL, *version = NULL; | ||
| ddtrace_span_data *source = ddtrace_otel_attr_source_span(root); | ||
| datadog_populate_target_data_with_defaults(source, &service, &env, &version, get_DD_SERVICE(), get_DD_ENV(), get_DD_VERSION()); |
There was a problem hiding this comment.
It's needed but it can be simplified to use this helper datadog_populate_target_data which already exists:
static inline void datadog_populate_target_data(ddtrace_span_data *span, zend_string **service, zend_string **env, zend_string **version) {
datadog_populate_target_data_with_defaults(span, service, env, version, get_DD_SERVICE(), get_DD_ENV(), get_DD_VERSION());
}AI explanation below.
This call resolves the effective Universal Service Monitoring identity written into the fixed-size OTel Thread Context record:
datadog_populate_target_data_with_defaults(...)It is needed because:
- service.name, deployment environment, and service version may come from span properties or tracer configuration.
- A nested local root should inherit these attributes from the entrypoint root rather than publishing its own integration service. This is why ddtrace_otel_attr_source_span() is used.
- There may be no applicable entrypoint span, in which case configured/default values are required.
- The record must contain copied bytes because an external profiler cannot safely dereference Zend strings.
This behavior is visible in otel_thread_context_stack_switch.phpt: otherRoot has other-service, but its Thread Context intentionally publishes main-service.
| offset = ddtrace_otel_record_write_attr_zstr(record, offset, DDTRACE_OTEL_ATTR_SERVICE_VERSION, ddtrace_otel_attr_zstr(version)); | ||
|
|
||
| char thread_id[32]; | ||
| int thread_id_len = snprintf(thread_id, sizeof(thread_id), "%llu", (unsigned long long)syscall(SYS_gettid)); |
There was a problem hiding this comment.
should we cache the tid vs doing a syscall on the write?
| #ifdef __linux__ | ||
| if (stack->active) { | ||
| ddtrace_update_otel_thread_context_span_id(SPANDATA(stack->active)); | ||
| } else { | ||
| ddtrace_otel_detach(); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This code should not be executed when a ddtrace_switch_span_stack happens later, otherwise it will be transiently detached before it switches to a parent stack.
if (&stack->root_span->span == span) {
ddtrace_switch_span_stack(stack->parent_stack);
stack->root_span = NULL;
} else if (!stack->active || SPANDATA(stack->active)->stack != stack) {
dd_close_entry_span_of_stack(stack);
} else {
#ifdef __linux__
if (stack->active) {
ddtrace_update_otel_thread_context_span_id(SPANDATA(stack->active));
} else {
ddtrace_otel_detach();
}
#endif
}
like this. Closing suffers from the same issue. dd_close_entry_span_of_stack also needs a detach if it's not switching spans.
| // TracerMetadata emits empty resource attributes for absent optional fields to advertise | ||
| // support. These values can vary independently on every PHP request, so their values and | ||
| // resource keys must be omitted from the process-wide context. The keys remain discoverable | ||
| // through threadlocal.attribute_key_map. | ||
| if let Some(resource) = context.resource.as_mut() { | ||
| resource.attributes.retain(|attribute| { | ||
| !matches!( | ||
| attribute.key.as_str(), | ||
| "service.name" | "service.version" | "deployment.environment.name" | ||
| ) | ||
| }); | ||
| } |
There was a problem hiding this comment.
would be better to change libdatadog not to add these when it's not provided
| DEPENDS ${CMAKE_SOURCE_DIR}/../datadog-linux.sym | ||
| VERBATIM | ||
| ) | ||
| elseif(APPLE) |
There was a problem hiding this comment.
I don't think this will build on mac os as is, as it appears to include linux only sources.
And the linux part doesn't add -mtls-dialect=gnu2.
But not critical as I promised the cmake ddtrace build would not be a maintenance burden :p
| if (datadog_sidecar_instance_id) { | ||
| ddog_sidecar_instanceId_drop(datadog_sidecar_instance_id); | ||
| datadog_sidecar_instance_id = NULL; | ||
| } | ||
| dd_set_resettable_sidecar_globals(); |
There was a problem hiding this comment.
After this, datadog_force_new_instance_id becomes dead code (only called from a test function). Why not just make datadog_force_new_instance_id unconditional and call it in the beginning of datadog_internal_handle_fork ?
| } | ||
|
|
||
| fn refresh(&mut self) -> std::io::Result<()> { | ||
| let result = ProcessContextSelfReader::new().and_then(|reader| reader.read()); |
There was a problem hiding this comment.
It should not be necessary to create a new ProcessContextSelfReader unless there was a fork or unpublish + publish. To do so, incurs the penalty of having to do discovery again, and calling pipe2 again.
There was a problem hiding this comment.
It's not particularly performance sensitive. There are two call sites:
- In RINIT, it's called from
initializeif there isn't a cached process context already. You can think of this as once per thread. - If there is an unknown thread attribute key, then we refresh the process context. In practice this should not happen, all of our keys are static at the moment. The exception is after a fork, as it gets invalidated during the pre-fork (this means that the parent also is invalidated).
Doing it this way means we don't hold open 2 file descriptors per thread. The thing cannot be shared across threads either.
In other words, aside from bugs, it's already almost optimal. If customers have fork-heavy code, we may want to revise this pre-fork part though.
| }; | ||
| if cache.refresh().is_err() { | ||
| return decoded; | ||
| } |
There was a problem hiding this comment.
If this starts failsing we presumably keep getting unknown_index == true, and we try again, with discovery and everything. I don't think it should retry indefinitely.
There was a problem hiding this comment.
I addressed this in 2c1a93d, give it a look!
| let Ok(cache) = cell.try_borrow() else { | ||
| return ThreadContext::default(); | ||
| }; | ||
| let (decoded, unknown_index) = cache.decode_thread_attributes(attributes, defaults); |
There was a problem hiding this comment.
Have you considering caching also the decoded ThreadContext. Presumably the cache would be hit most of the time.
| fn effective_profile_tags(base: Arc<Vec<Tag>>, identity: &ProcessIdentity) -> Arc<Vec<Tag>> { | ||
| let mut updated = None; | ||
| apply_profile_tag_override(&base, &mut updated, "service", identity.service.as_deref()); | ||
| apply_profile_tag_override(&base, &mut updated, "env", identity.environment.as_deref()); | ||
| apply_profile_tag_override(&base, &mut updated, "version", identity.version.as_deref()); | ||
| updated.map(Arc::new).unwrap_or(base) | ||
| } |
There was a problem hiding this comment.
TODO: I think the performance of this function is going to be a bit poor. Should probably not emit the service/env/version tags and "merge" things at a higher level, rather than updating tags.
| void datadog_otel_process_context_publish(void) { | ||
| char detected_hostname[HOST_NAME_MAX + 1] = {0}; | ||
| ddog_CharSlice hostname = {0}; | ||
| if (gethostname(detected_hostname, HOST_NAME_MAX) == 0) { |
There was a problem hiding this comment.
The gethostname() function returns the standard host name for the current processor, as previously set by sethostname(). The namelen argument specifies the size of the name array. The returned name is null-terminated, unless insufficient space is provided.
Host names are limited in length to {sysconf(_SC_HOST_NAME_MAX)} characters, not including the trailing null, currently 255.
So you should pass sizeof(detected_hostname) / HOST_NAME_MAX + 1 to gethostname.
| const zend_extension *maybe_ddtrace = (zend_extension *)item->data; | ||
| if (maybe_ddtrace != extension && is_ddtrace_extension(maybe_ddtrace)) { | ||
| #ifdef __linux__ | ||
| datadog_php_profiling_ddtrace_handle = maybe_ddtrace->handle; |
There was a problem hiding this comment.
I don't think the ssi loader sets this, which would cause datadog_php_profiling_get_otel_thread_context to return early.
3 participants
PROF-15487
Description
Publish standard OTel process and thread contexts from the tracer and make the profiler consume them for runtime identity and effective service metadata. Handle span, stack, Fiber, configuration, and fork lifecycle changes while retaining the legacy non-Linux path.
WIP for appsec.
Reviewer checklist