Repository navigation
[SVLS-9300] add instrumentation configuration flags for ecs-fargate instrument - #2465
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: ac06b03 | Docs | View more details | Give us feedback! |
ojproductions
changed the title
ojproductions
added
serverless
|
@codex review |
![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: 5c3513235a
ℹ️ 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".
| [ECS_FARGATE_ENV_VAR]: 'true', | ||
| [SITE_ENV_VAR]: settings.site, | ||
| // The Agent's own trace intake, which is a separate switch from the tracers' `DD_TRACE_ENABLED`. | ||
| [DD_APM_ENABLED_ENV_VAR]: String(settings.tracing ?? true), |
There was a problem hiding this comment.
When --tracing false is combined with --llmobs, this sets DD_APM_ENABLED=false on the sidecar while the application is configured with DD_LLMOBS_AGENTLESS_ENABLED=false, explicitly requiring the sidecar to forward its LLM Observability payloads. The resulting task cannot deliver those payloads; either keep the Agent intake enabled when an Agent-backed product is selected or reject this option combination.
Useful? React with 👍 / 👎.
| const appEnvironment = getAppContainerEnvVars(settings, family) | ||
| const appLabels = getUstDockerLabels(settings, family) |
There was a problem hiding this comment.
When an application container already declares DD_SERVICE but has no Datadog service Docker label, getAppContainerEnvVars() preserves that value while getUstDockerLabels() independently defaults the label to the task family. For example, a container with DD_SERVICE=checkout in family my-app is registered with com.datadoghq.tags.service=my-app, so its traces and Agent-collected container metrics are assigned to different services. Derive the default label from the container's effective DD_SERVICE, or avoid adding the family label when the container has chosen its own service.
Useful? React with 👍 / 👎.
5c35132 to
828a074
Compare
2aae581 to
1d41bbb
Compare
ava-silver
left a comment
ava-silver
left a comment
There was a problem hiding this comment.
Automated AI Review (human curated)
The command needs a consistent desired-state model for UST, product settings, and log collection. I also noted one documentation mismatch.
|
|
||
| const containers = taskDefinition.containerDefinitions ?? [] | ||
| const borrowed = borrowedLogConfiguration(containers) | ||
| const firelens = settings.logCollection ? firelensLogConfiguration(settings) : undefined |
There was a problem hiding this comment.
When log collection is turned off, firelens becomes undefined and the existing router plus every existing awsfirelens configuration remain unchanged. The command therefore cannot converge back to its default log state, and no ECS uninstrument command exists to clean these CLI-managed artifacts. Add a cleanup path that removes the router and its routing configuration.
|
|
||
| if (settings.service) { | ||
| managed[SERVICE_ENV_VAR] = settings.service | ||
| } else if (family) { |
There was a problem hiding this comment.
Using the family as a default here preserves an existing application DD_SERVICE, but the Agent and revision tag use the family. A task with DD_SERVICE=checkout in a payments-worker family then emits traces as checkout and Agent/resource telemetry as payments-worker. Resolve UST once, then apply the same desired state to every Datadog-owned UST destination.
| if (settings.tracing !== undefined) { | ||
| managed[DD_TRACE_ENABLED_ENV_VAR] = String(settings.tracing) | ||
| } | ||
| if (settings.appsec) { |
There was a problem hiding this comment.
--no-appsec resolves to false, but this add-only branch leaves an existing DD_APPSEC_ENABLED=true untouched. The same desired-state issue applies to disabling source-code integration and omitting other new settings. Resolve each setting to an enabled value or absence, then apply or remove its Datadog-owned fields consistently.
| }) | ||
| private envVars = Option.Array('-e,--env-vars', { | ||
| description: | ||
| 'Additional environment variables to set on every container in the task. Can specify multiple variables in the format `--env-vars VAR1=VALUE1 --env-vars VAR2=VALUE2`.', |
There was a problem hiding this comment.
This says every container, but with --log-collection the transform deliberately excludes datadog-log-router from envVars. Please describe this as applying to application containers and the Datadog Agent.
bdb2fd9 to
7177257
Compare
7177257 to
027a089
Compare
9ceffa4 to
8a010fd
Compare
8a010fd to
15b0eae
Compare
15b0eae to
d2d53ec
Compare
d2d53ec to
4c7047f
Compare
00803c3 to
2742bd9
Compare
2742bd9 to
110bddd
Compare
110bddd to
ac06b03
Compare
3 participants
What and why?
Adds the flags that decide how an instrumented task reports to Datadog. Without them the
command produces telemetry that is hard to slice: everything lands under the task
definition family with no environment or version to correlate against.
How?
Unified service tagging (
--service,--env,--version,--extra-tags) is writtenthree ways, so the same task is named identically wherever it shows up:
DD_SERVICE,DD_ENV,DD_VERSION, andDD_TAGSon every container, which iswhat the tracers send;
com.datadoghq.tags.*Docker labels on the application containers, which is whatthe Agent reads to tag the metrics it collects about them from the outside. The Agent
container is deliberately left unlabelled, so it does not report its own resource usage
under your service;
An explicit flag overrides what a container already declares; the task definition family
is only a fallback, and
DD_SERVICE,DD_TRACE_ENABLED, andDD_LOGS_INJECTIONarefilled in only when the container has not made a choice itself.
Also the product toggles the tracers read (
--tracing,--log-level,--appsec,--llmobs,--env-vars) and the source code integration. Its Git tags are resolved evenon a dry run so the diff shows the
DD_TAGSa real run would write, while the metadataupload — a write to Datadog — is skipped.
Review checklist