fix(library-config): omit absent process context attributes - #2337
Conversation
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 96f7f8e | Docs | Datadog PR Page | Give us feedback! |
|
The original implementation did drop undefined attributes, but @ivoanjo reverted this decision explicitly, as per this review comment: #1658 (comment), and as hinted at by the presence of the comment. It could be good to double check with him that this is ok. |
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
|
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
Thanks for the ping @morrisonlevi and @yannham . I think for the multi-tenant use-case this change is fine, and for all others, our libraries usually have defaults for most of them so in practice I don't think this empty vs omitted will show up very often there.
| key_value_if_some("container.id", container_id), | ||
| ] | ||
| .into_iter() | ||
| .flatten() | ||
| .collect(); | ||
|
|
||
| // `mut` is only needed when `otel-thread-ctx` is enabled. | ||
| #[allow(unused_mut)] | ||
| let mut extra_attributes = vec![key_value_opt("datadog.process_tags", process_tags)]; | ||
| let mut extra_attributes: Vec<_> = key_value_if_some("datadog.process_tags", process_tags) | ||
| .into_iter() | ||
| .collect(); |
There was a problem hiding this comment.
I agree that for a multi-tenant case (like we're doing for PHP), it's kinda weird to have empty service.name/service.instance.id/service.version/deployment.environment.name since those are properties of a given tenant, not for the whole process.
Yet, container.id and datadog.process_tags I think still make sense as being process-wide.
Do you mind if we keep the old key_value_opt for those two, and use the new key_value_if_some only for those that are truly per-tenant?
There was a problem hiding this comment.
For container.id, I agree. Sadly process tags are service specific with the svc tags...
There was a problem hiding this comment.
👍 I wasn't aware of that, thanks for catching it!
There was a problem hiding this comment.
Given that I don't think it makes much sense to have particular behaviour for just this single container.id, I'm now just merging this as is.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
5 participants
What does this PR do?
If these items are not set, they should be missing, rather than sent with no value.
Motivation
In PHP, these will not be set at all because they live in the OTel thread context instead of the process context.
Additional Notes
Needed for DataDog/dd-trace-php#4077.
How to test the change?
Tests the same, there just won't be "empty" items and they'll be missing instead.