Conversation
dd-octo-sts
Bot
added
area/components
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new static-tag path overwrites otlp_config.metrics.tags instead of preserving configured metric tags. Any deployment that combines existing OTLP metric tags with provider_kind or EKS Fargate tagging will silently lose those existing tags, changing metric dimensions and potentially breaking dashboards and monitors.
🤖 Datadog Autotest · Commit 6e25d0d · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| let mut otlp = otlp.clone(); | ||
| let static_tags = resolve_static_metric_tags(&otlp, &shared.tags, |key| std::env::var(key).ok()); | ||
| if !static_tags.is_empty() { | ||
| otlp.metrics.tags = static_tags.join(","); |
There was a problem hiding this comment.
Existing OTLP metric dimensions silently disappear for users adopting provider-kind or EKS Fargate tagging, causing incorrect grouping and broken dashboards or monitors.
Assertion details
- Input: Configure
otlp_config.metrics.tagsasservice:checkoutand enable the newprovider_kind:ekssetting.resolve_static_metric_tagsreturnsprovider_kind:eks, after whichfrom_configurationassigns that result tootlp.metrics.tags. - Expected: Configured metric tags remain present and the new static tags are added, producing
service:checkoutandprovider_kind:eks. - Actual: The assignment at line 129 replaces the entire configured tag string with
provider_kind:eks;service:checkoutis absent before the metric translator parses the tags.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| otlp.metrics.tags = static_tags.join(","); | |
| otlp.metrics.tags = if otlp.metrics.tags.is_empty() { | |
| static_tags.join(",") | |
| } else { | |
| format!("{},{}", otlp.metrics.tags, static_tags.join(",")) | |
| }; |
There was a problem hiding this comment.
This is addressed in the comment below. The Codex bot is effectively pointing out the same thing.
![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: 6e25d0d689
ℹ️ 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".
| let static_tags = resolve_static_metric_tags(&otlp, &shared.tags, |key| std::env::var(key).ok()); | ||
| if !static_tags.is_empty() { | ||
| otlp.metrics.tags = static_tags.join(","); |
There was a problem hiding this comment.
When provider_kind is non-empty or the process is detected as ECS/EKS Fargate, static_tags is non-empty and this assignment discards the existing otlp_config.metrics.tags value. Since build() later parses only self.otlp.metrics.tags, every explicitly configured OTLP tag silently disappears in those environments; merge or append the static tags instead of replacing the configured string.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That is intentional parity as seen here. We're correctly overwriting otlp_config.metrics.tags when resolved static tags are non-empty.
| /// Static provider classification added to every OTLP metric when non-empty. | ||
| pub provider_kind: String, | ||
|
|
||
| /// Whether the Agent runs as an EKS Fargate sidecar. | ||
| pub eks_fargate: bool, |
There was a problem hiding this comment.
The newly added Metrics fields only summarize their purpose, and the added GlobalTags fields similarly only name their source; none explicitly states the empty/false default, relevant boundary behavior, or who should change the setting. Expand the Rustdoc for every added configuration field to cover those required details.
AGENTS.md reference: AGENTS.md:L149-L154
Useful? React with 👍 / 👎.
| tags.into_shared() | ||
| } | ||
|
|
||
| /// Resolves static tags that the Core Agent adds to OTLP metrics. |
There was a problem hiding this comment.
This new Rustdoc names the Core Agent inside lib/saluki-components, which is a generic Saluki crate whose documentation and comments must avoid Datadog Agent-specific names and topology. Describe these as static tags supplied by the server process or configuration instead.
AGENTS.md reference: AGENTS.md:L127-L136
Useful? React with 👍 / 👎.
Binary Size Analysis (Agent Data Plane)Baseline: ff0b440 · Comparison: 375a82e · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
| if !otlp.metrics.kubernetes_kubelet_nodename.is_empty() { | ||
| tags.push(format!("eks_fargate_node:{}", otlp.metrics.kubernetes_kubelet_nodename)); | ||
| } else { | ||
| warn!("Couldn't build the 'eks_fargate_node' tag: kubernetes_kubelet_nodename is not configured."); |
There was a problem hiding this comment.
Is this log message (and the equivalent one below) meant to explicitly match a log message from the Core Agent?
It might be good to try and reword it to make it more friendly to users, in a "problem-solution" pattern:
Tag 'eks_fargate_node' will be missing from telemetry ingested via OTLP due to missing configuration data. Ensure 'kubernetes_kubelet_nodename' is set in the configuration.
If this is just something they can set (but more on that in a second), then we should explicitly be telling them not only what to set but where to set it. As is, we're not being that explicit that this is a configuration setting.
Separately: is this actually something users are expected to set manually, or something that would be configured by the Helm chart or Operator? If it's the latter, then we actually might not want to have the "solution" part if there's no way for them to act on it.
There was a problem hiding this comment.
Seems like it's something that users set manually. I do think the problem-solution pattern for the error message is better than what I had and was added in c299b79 😄
| let ecs_fargate = | ||
| environment("AWS_EXECUTION_ENV").as_deref() == Some("AWS_ECS_FARGATE") || environment("ECS_FARGATE").is_some(); |
There was a problem hiding this comment.
This is bothering me. It's an environmental side-effect buried in an otherwise relatively pure function. I feel like this check belongs somewhere that is dedicated to setup (right?) and that its environment determination should be passed in to this function. I'm not sure if there's a "place" for that, but it jumped out at me as a kind of hidden form of config/environment sensitivity.
There was a problem hiding this comment.
You're right. Fixed in c299b79. I made it such that the check for ECS_Fargate occurs much earlier and is propagated through instead of belonging to that hacky conditional statement you called out.
There was a problem hiding this comment.
As I was looking at this, the major question that came up in my mind, which I'm having trouble answering is: why did it come up now that we need the tags and extra_tags configuration fields when we did not need these for DogStatsD.
My limited understanding of this is, ADP's HostTagsConfiguration is temporary because normally it's handled by the backend, but a window is needed before the Agent can sent the host and host_tags to the backend. After that, the backend associates those host tags with data from the host.
These tags and extra_tags fields feel suspiciously related to that mechanism, and seemingly, to me, would behave the same way for OTLP-sourced data in the backend as they would for DSD-sourced data.
To state the halting point I have on this PR differently, or again...
It feels like either this behavior is needed for DogStatsD as well as OTLP, or that it is needed by neither of them. It is entirely possible that I am missing something though because this is a very confusing area of Agent/ADP contract.
Edit: After reading the above, AI is helpfully pointing out that:
OTLP could legitimately differ if its metrics bypass the mechanism by which backend host association works for DogStatsD. That is probably the exact question you want need to answer.
| } | ||
|
|
||
| /// Returns whether the process runs in an Amazon ECS Fargate environment. | ||
| pub fn is_ecs_fargate() -> bool { |
There was a problem hiding this comment.
Based on what else is in this file, such as "running in docker", this does seem like a reasonable place to put this function.
| self.config.domains.dogstatsd.listeners.provider_kind = value.clone(); | ||
| self.config.domains.otlp.metrics.provider_kind = value; |
There was a problem hiding this comment.
This is a bit suspicious. I wonder if there is a better home for it in config.shared. I recall that early in the typed config process it was considered desirable that we avoid duplication of a value into two different places in the typed config structure.
lucastemb
changed the title
a1964c3 to
11fa9b2
Compare
3 participants
Summary
Added
provider_kindsupport and EKS Fargate related static tags as part of the ongoing OTLP Ingest metrics pipeline related work.Support will be pipeline agnostic (meaning it will also apply to DogStatsD) since investigation reveals that this is not scoped simply to just the OTLP pipeline.
Change Type
How did you test this PR?
Unit tests.
References