-
Notifications
You must be signed in to change notification settings - Fork 11
[CONTP-1014] feat: Add UST docker labels to all container definitions #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| dockerLabels = merge( | ||
| local.ust_docker_labels, | ||
| // Placing this after local.ust_docker_labels ensures user defined UST labels are not overwritten. | ||
| lookup(container, "dockerLabels", {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other application container configured options like environment variables, mount points, and container dependencies give more precedence to the values defined by the Datadog Terraform module.
I think we should be fine to maintain that level of precedence here and assume that if a user defined the UST values for env, service, and version then those values would take precedence as well.
terraform-aws-ecs-datadog/modules/ecs_fargate/variables.tf
Lines 117 to 133 in 1419f73
| variable "dd_service" { | |
| description = "The task service name. Used for tagging (UST)" | |
| type = string | |
| default = null | |
| } | |
| variable "dd_env" { | |
| description = "The task environment name. Used for tagging (UST)" | |
| type = string | |
| default = null | |
| } | |
| variable "dd_version" { | |
| description = "The task version name. Used for tagging (UST)" | |
| type = string | |
| default = null | |
| } |
We can leave a bugfix note in the changelog for the next release to notify users that the UST tagging has been fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understandable. I guess if they wanted unique UST values, then they shouldn't set the global env,service,and version. They should then manually set it per container definition.
I initially wanted to prioritize the user's docker labels over ours because the customer's problem description implied the need for different UST values.
modules/ecs_fargate/datadog.tf
Outdated
|
|
||
| dd_agent_docker_labels = merge( | ||
| local.ust_docker_labels, | ||
| var.dd_docker_labels, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if we should be setting the UST tags on the Agent container the same as the application containers. We should get some other opinions about this and compare what our approach is for EKS Fargate with the admission controller on the DCA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already set UST env vars, albeit they don't work for the agent.
The DCA agent sidecar mutator doesn't inject the agent with any UST tags, so I think It makes sense to remove these UST tags from the terraform agent as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the UST env vars on the Agent (and also fluentbit and cws-init) are actually doing nothing. We should be fine clean up and remove them.
What does this PR do?
Adds UST docker labels to all app container definitions excluding agent containers.
configuration to docker label mapping:
dd_env->com.datadoghq.tags.envdd_service->com.datadoghq.tags.servicedd_version->com.datadoghq.tags.versionExposes configurable docker labels on agent containers through
dd_docker_labels.Motivation
ecs.fargate.cpu.limitand similar metrics don't haveservice/env/versiontags despite setting UST env vars on all container environments. That's because UST tags cannot be created based on environment variables when deploying on ECS Fargate.Adding UST docker labels fixes this such that metrics and logs are properly tagged with
service/env/version.Describe how you validated your changes
ust_docker_labels_test.go.Manual Testing
dd_service/dd_version/dd_envsetup.ecs.fargate.cpu.limitmetrics are tagged with UST tags.Additional Notes