feat(otel-thread-ctx)!: add self check capability#2095
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: b403a00 | Docs | Datadog PR Page | Give us feedback! |
yannham
changed the title
yannham
changed the title
![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: 06bb468de7
ℹ️ 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".
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
|
I'm curious -- how would this interact with #2129 ? |
|
#2129 should be an internal and feature-preserving refactoring that doesn't change anything from the reader side, so the implementation of this sanity check doesn't depend in any way on whether #2129 is used or not (we basically just read our own elf like an external reader), and #2129 should ideally pass the sanity check once it lands as well. |
yannham
force-pushed
the
yannham/thread-ctx-autocheck
branch
from
02b5f05 to
804db1d
Compare
yannham
force-pushed
the
yannham/thread-ctx-autocheck
branch
from
804db1d to
84b36fe
Compare
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
👍 LGTM this looks good! Thanks for the patience with my extremely slow review 😅
It might be worth getting a second quick pass from @scottgerring or @nsavoire since they've been spending a lot more time on the elf details than I have, but I'd say not a blocker (e.g. they can give a pass after we merge too)
scottgerring
left a comment
scottgerring
left a comment
There was a problem hiding this comment.
cool idea!
couple nits but lgtm
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: waiting
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
…SDESC The linker can optimize TLSDESC to Local Exec, so a positive TLSDESC check is too strict. Instead, assert that no General Dynamic or Local Dynamic relocations (DTPMOD/DTPOFF) are present for otel_thread_ctx_v1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract check_tls_slot_in(path) from check_tls_slot_present() so the integration test can call the same programmatic checks instead of shelling out to readelf. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous check only forbade GD/LD relocations (DTPMOD/DTPOFF), missing other TLS relocation types like TPOFF64 or GOTTPOFF. Flip from a blocklist to an allowlist: only TLSDESC is accepted. Rename check_no_gd_ld_reloc -> check_tlsdesc_reloc_only accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yannham
force-pushed
the
yannham/thread-ctx-autocheck
branch
from
d7f067a to
ddf086c
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
Tests failed on this commit 92480d3: What to do next?
|
|
@yannham I'm going to go ahead and re-add this to the merge queue, hope that's fine with you ;) |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
yannham/thread-ctx-autocheck
branch
## chore: release v37.0.0 Bumps the workspace version from `36.0.0` to `37.0.0` in `Cargo.toml` and regenerates `Cargo.lock`. ## Post-merge steps 1. Trigger the `create_release` job on GitLab — builds artifacts and creates a draft GitHub release. 2. Ask someone from `libdatadog-core` or `libdatadog-release` to publish the draft release. 3. Trigger the `release-proposal-dispatch` GitHub Actions workflow for per-crate crates.io publishing: crates: libdd-data-pipeline,libdd-trace-stats,libdd-trace-utils,libdd-ddsketch ## Notable changes since v36.0.0 - `feat(data-pipeline)`: export client-computed span stats as OTLP trace metrics (#2067) - `feat(otel-thread-ctx)`: add self check capability (#2095) - `fix(trace-stats)`: add `grpc_method` to aggregation key (#2151) - `feat(data-pipeline)`: add stdout log trace exporter (#2074) - `feat(remote-config)`: use the proto file from the agent (#2165) - `feat(sidecar)`: expose `default_service_name` for `svc.*` process tags (#2053) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) Co-authored-by: munir.abdinur <munir.abdinur@datadoghq.com>
3 participants
What does this PR do?
This PR adds a self-test capability to libdd-otel-thread-ctx (and to the FFI), for both shared libraries and statically linked executables. The test checks that the TLS symbol is visible in the dynsym table of the library, and that the model is TLSDESC
Motivation
OTel thread context requires a specific linking setup, which if not met, makes the context non-discoverable and thus the whole feature unusable. It's thus useful, at least for the CI and post-build sanity checks of downstream profilers to be able to verify that a build is functional.
Additional Notes
The PR is feature-gated so that the
readelfdependency isn't pulled if the check is not useful (typically, it can be peformed at build time but be skipped from the final tracer/profiler if desired).How to test the change?
The existing test has been refactored to use that code (instead of commands). I've checked that removing the TLSDESC dialect configuration option from the linker, or removing the dynamic list which doesn't export the symbol, make the test fail with the expected error message.