feat!: expose ThreadLocalMetadata via process_discovery#153
Conversation
Replaces the flat `threadlocal_attribute_keys` field on the napi TracerMetadata with a `threadlocal_metadata` substruct mirroring the libdatadog Rust API. Lets callers set the schema-version string and extra process-context attributes (strings and 64-bit ints for now) alongside the attribute key map. libdd-library-config and libdd-trace-protobuf are pinned to the merge commit that introduced the new API (7cdeb7896e92d1ba38bde495934e112dac2eda25); swap back to a tagged release once one that includes it is published. This is a breaking change to the process_discovery.TracerMetadata constructor. Downstream (dd-trace-js) will need to migrate from passing threadlocalAttributeKeys directly to passing a threadlocalMetadata object.
Overall package sizeSelf size: 28.12 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------|🤖 This report was automatically generated by heaviest-objects-in-the-universe |
| /// Extra `threadlocal.*` attributes to publish alongside the key map (e.g. | ||
| /// V8 layout constants a Node.js reader needs to walk from the discovery | ||
| /// TLS symbol into the record). | ||
| pub extra_attributes: Vec<ExtraAttribute>, |
There was a problem hiding this comment.
I would think an empty vec is ok here, unless you really need to differentiate None from Some(vec![]) for some reason. Or do you ask with respect to null support or something related to JS-Rust interop?
| } | ||
|
|
||
| fn convert_extra_attribute(ea: &ExtraAttribute) -> Option<(String, any_value::Value)> { | ||
| let value = if let Some(s) = &ea.string_value { |
There was a problem hiding this comment.
The doc on line 16-17 say set exactly one but this function prefers string value. Should this be surfaced or is this okay?
There was a problem hiding this comment.
Maybe the API contract is the doc 🤷, but just noting that the program itself has behavior that is not surfaced just by "set exactly one"
There was a problem hiding this comment.
Oh you mean how we aren't checking for when both are set? Yeah, it'd be polite to throw an error instead of ignoring the int value. I'll do that.
| pub string_value: Option<String>, | ||
| pub int_value: Option<i64>, |
There was a problem hiding this comment.
If we should only have one, then this should be an enum.
There was a problem hiding this comment.
Ah, I suppose because it has to be mapped to a JS object? In that case please ignore my comment.
| /// Extra `threadlocal.*` attributes to publish alongside the key map (e.g. | ||
| /// V8 layout constants a Node.js reader needs to walk from the discovery | ||
| /// TLS symbol into the record). | ||
| pub extra_attributes: Vec<ExtraAttribute>, |
There was a problem hiding this comment.
I would think an empty vec is ok here, unless you really need to differentiate None from Some(vec![]) for some reason. Or do you ask with respect to null support or something related to JS-Rust interop?
Per review feedback: the previous behavior silently preferred string_value when both were set and silently dropped the entry when neither was — both are shapes a caller likely didn't mean. Return an InvalidArg napi::Error instead, matching the 'exactly one of' docstring. Test both paths.
3 participants
Summary
Exposes the new
ThreadLocalMetadatashape from libdatadog'slibdd-library-config(introduced in DataDog/libdatadog#2162) viaprocess_discovery, so tracers building against libdatadog-nodejs can drive the fullthreadlocal.*block of the OTel process context — not just the attribute key map.Changes
crates/process_discovery/Cargo.tomllibdd-library-configfromtag = "v35.0.0"torev = "7cdeb7896e92d1ba38bde495934e112dac2eda25"(the merge commit of libdatadog#2162). Swap back to a tagged release once one that includes this commit is published.libdd-trace-protobuf(at the same rev) to reachany_value::Valuewhen converting caller-supplied extras.crates/process_discovery/src/lib.rsthreadlocal_attribute_keys: Option<Vec<String>>field on the napiTracerMetadatawiththreadlocal_metadata: Option<ThreadLocalMetadata>.#[napi(object)]types mirroring the upstream shape:ThreadLocalMetadata { attribute_keys, schema_version, extra_attributes }— drives thethreadlocal.*block;null/omitted disables the block entirely.ExtraAttribute { key, string_value?, int_value? }— one KeyValue for the process context. Exactly one ofstring_value/int_valueshould be set; entries with neither are silently dropped. Only string and 64-bit int are exposed for now — the otherAnyValuevariants (bool, double, bytes, array, kvlist) can be added later as needed.Test plan
yarn build-debug && node test/process-discovery.jslocallydd-trace-js— the tracer-metadata construction there needs to switch from passingthreadlocalAttributeKeysto athreadlocalMetadataobjectFollow-up
Once libdatadog cuts a release that includes 7cdeb7896e92d1ba38bde495934e112dac2eda25, replace the git-rev deps with a tagged version.
Jira: PROF-15221