feat(library-config)!: caller-supplied threadlocal schema and extra process-context attributes#2162
feat(library-config)!: caller-supplied threadlocal schema and extra process-context attributes#2162szegedi wants to merge 5 commits into
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 6e826b1 | Docs | Datadog PR Page | Give us feedback! |
📚 Documentation Check Results📦
|
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. |
🔒 Cargo Deny Results📦
|
szegedi
force-pushed
the
szegedi/threadlocal-schema-and-extras
branch
from
c22f97e to
9543321
Compare
szegedi
changed the title
szegedi
force-pushed
the
szegedi/threadlocal-schema-and-extras
branch
from
9543321 to
ce85ea7
Compare
szegedi
changed the title
…ributes The OTel process-context block emitted by TracerMetadata::to_otel_process_ctx used to hardcode 'threadlocal.schema_version' to 'tlsdesc_v1_dev' and offered no way to add further threadlocal.* attributes. Language-specific writers (e.g. a Node.js writer that publishes its own V8 layout constants under a different schema name) couldn't fully drive the process context through this path. Add two new fields on TracerMetadata, gated on the otel-thread-ctx feature: - threadlocal_schema_version: Option<String> — explicit override of the schema attribute. Falls back to 'tlsdesc_v1_dev' when None and threadlocal_attribute_keys is Some, preserving backward-compatible behavior for existing callers. - threadlocal_extra_attributes: Vec<(String, ProcessContextAttrValue)> — additional KeyValues emitted in the process context. The value type is a small enum (String or Int) covering the cases needed for writer layout / runtime metadata. Emission of each piece is now independent: the schema attribute, the key map, and each extra are emitted iff their source field is set.
szegedi
force-pushed
the
szegedi/threadlocal-schema-and-extras
branch
from
ce85ea7 to
79d90e9
Compare
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
|
![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: 79d90e9755
ℹ️ 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".
| @@ -132,7 +171,9 @@ impl TracerMetadata { | |||
| if let Some(threadlocal_attribute_keys) = threadlocal_attribute_keys.as_ref() { | |||
There was a problem hiding this comment.
When a caller sets threadlocal_schema_version or threadlocal_extra_attributes but has no additional attribute keys to publish, this guard skips the whole block and silently drops the supplied schema/layout attributes. That prevents the new caller-supplied process-context fields from working unless callers also know to set threadlocal_attribute_keys to Some(vec![]), even when they do not need a key map; gate the schema, key map, and extras independently so provided extras are actually published.
Useful? React with 👍 / 👎.
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
The motivation LGTM. I wonder though if we shouldn't gather all thread-locals related stuff in a substruct, something like:
#[cfg(feature = "otel-thread-ctx")]
ThreadLocalMetadata {
threadlocal_attribute_keys: Vec<String>,
threadlocal_schema_version: Option<String>,
threadlocal_extra_attributes: Vec<(String, ProcessContextAttrValue)>,
}
And then have in TracerMetadata a single field
#[cfg(feature = "otel-thread-ctx")]
#[serde(skip)]
pub threadlocal_metadata: Option<ThreadLocalMetadata>,
The idea being that this avoid illegal states that are currently possible, like threadlocal_extra_attributes or threadlocal_schema_version being defined/non-empty but with attribute_keys being empty, which results in silently not publishing a ProcessContext.
With the ThreadLocalMetadata, the single point of activation is whether threadlocal_metadata is Some or not. So it's not possible to have it halfway defined.
This would be breaking but is ok - in practice the FFI is the only one currently using this code, so it's all within libdatadog, to the best of my knowledge.
What do you think?
| /// Value of an additional OTel process-context attribute. Mirrors the small subset of | ||
| /// `opentelemetry::proto::common::v1::AnyValue` variants we support for caller-supplied threadlocal | ||
| /// extras — string and 64-bit integer, since the only consumers so far are textual schema | ||
| /// identifiers and small numeric layout constants (e.g. struct offsets, pointer widths). |
There was a problem hiding this comment.
Are we confident this will always be the case? I feel a simple approach would just to let caller set any value supported by the process context spec (and that would get rid of this additional type). Put differently, would that be a problem / redflag if libdatadog consumers passed something else than a string or an int?
It's ok to start like this and evolve in the future (although it's a breaking change), we don't have to envision all possible use cases right now. I'm asking mostly because it adds boilerplate, so wondering if it's really necessary.
There was a problem hiding this comment.
Fair enough, we can either go with Vec<(String, any_value::Value)> or even Vec<KeyValue> in which case the caller needs to construct KeyValue. For that reason, I'd lean towards Vec(String, any_value::Value) although admittedly I don't have enough idiomatic Rust experience to decide. Which one would you suggest?
|
(Another solution would be to remove the |
|
@yannham I like the suggestion of adding a struct a lot, I'm all for eliminating illegal states. I might remove the |
szegedi
force-pushed
the
szegedi/threadlocal-schema-and-extras
branch
2 times, most recently
from
dfe7a07 to
261edb3
Compare
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@datadoghq.com>
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@datadoghq.com>
Per review feedback: dropping the ProcessContextAttrValue enum (String + Int) avoids the "what about bool/double/array" extension problem and reuses the otel_proto type that's already part of this file's surface. The struct also has to drop its Eq+Hash derive, since any_value::Value contains f64 (no Eq) and arrays (no Hash); nothing in-tree uses TracerMetadata as a hash-map key, and PartialEq is preserved.
Per review: a single Option<ThreadLocalMetadata> field on TracerMetadata replaces the three loose threadlocal_* fields. The 'illegal state' of schema/extras set without a key map is now unrepresentable by construction. Field names inside the substruct drop the threadlocal_ prefix since the namespace lives in the type name.
szegedi
force-pushed
the
szegedi/threadlocal-schema-and-extras
branch
from
261edb3 to
6e826b1
Compare
|
I updated the PR with the suggestions. For one, switched to For another, I introduced the Apologies for force pushes after review; my clanker did that by amending into the original commit and force-pushing. I restored the original commit from reflog and then applied the changes on top again as separate commits, so even though it says a bunch of "force pushed" in the PR history, the first commit is again the exact same SHA that you originally reviewed. |
2 participants
Summary
TracerMetadata::to_otel_process_ctxused to hardcodethreadlocal.schema_version = "tlsdesc_v1_dev"and offered no way to publish additionalthreadlocal.*attributes. A language-specific writer (e.g. a Node.js writer with its own V8 layout constants under a different schema) couldn't fully drive the process context through this path.This PR introduces a
ThreadLocalMetadatasubstruct that gathers all thread-local context configuration in one place, and replaces the existingthreadlocal_attribute_keysfield onTracerMetadatawith a singleOption<ThreadLocalMetadata> threadlocal_metadata:attribute_keys— same role as the previousthreadlocal_attribute_keys. The first key is still implicitlydatadog.local_root_span_id.schema_version— value ofthreadlocal.schema_versionpublished in the process context. Falls back to"tlsdesc_v1_dev"whenNone, preserving the behavior current callers rely on.extra_attributes— additional KeyValues to publish in the process context, e.g. the V8threadlocal.wrapped_object_offset/threadlocal.tagged_sizelayout constants the Node.js reader needs. Values are typed directly asany_value::Value, so callers can use any of the OTel-supported variants (string, int, bool, double, bytes, array, kvlist).Option<ThreadLocalMetadata>is the single switch for whether thethreadlocal.*section appears at all. Illegal states like "schema_version set but no key map" are unrepresentable by construction.Backward compatibility
Adding fields to
ThreadLocalMetadata(rather than directly toTracerMetadata) means future extensions don't break exhaustive struct literals at call sites. Existingthreadlocal_attribute_keys: Some(vec![...])callers migrate tothreadlocal_metadata: Some(ThreadLocalMetadata { attribute_keys: vec![...], ..Default::default() })— that's a breaking change, hence the!in the PR title, but in practice the only in-tree consumer islibdatadog-nodejs.Also dropped
Eq, HashfromTracerMetadata's derive:any_value::Valuecontainsf64(noEq/Hash) and arrays (noHash). Nothing in-tree putsTracerMetadatain a hash-keyed collection;PartialEqis retained.Test plan
cargo test -p libdd-library-config --features otel-thread-ctxThreadLocalMetadatainlibdatadog-nodejs'sprocess_discoverycratedd-trace-jspass schema"nodejs_v1_dev"+ V8 layout constants through, dropping its own ad-hoc handling