Add setting container ID as UDP origin for Dogstatsd metrics - #9338
Add setting container ID as UDP origin for Dogstatsd metrics#9338kenpetti-toasttab wants to merge 2 commits into
Conversation
|
|
kenpetti-toasttab
changed the title
dd.internal.entity_iddd.internal.entity_id in Dogstatsd
kenpetti-toasttab
changed the title
dd.internal.entity_id in Dogstatsd…D based on the `dd.internal.entity_id` metric tag. * Added tests for the above * Updated tests to make the `kubernetes_pod_uid` values actually UUIDs rather than fake test data.
9095f8f to
40fc1bf
Compare
|
@djmitche I've added release notes, can you confirm that they look ok? Also, there are the tag-related checks (milestone, team_label) that I think you need to address, since I can't add tags. |
|
I'll leave both of those to the @DataDog/container-integrations team. My review is more of a lower-level "looks correct", while they can better answer "is this a good idea." |
ahmed-mez
left a comment
ahmed-mez
left a comment
There was a problem hiding this comment.
Thanks @kenpetti-toasttab for the contribution, I added a suggestion to make sure we don't break compatibility and make it possible to support UUIDs even outside of kubernetes in the future. Thanks!
| // All Kubernetes pod entityIDs are in UUID format. If the entityIDValueis a valid UUID, set | ||
| // k8sOrigin using the KubePodTaggerEntityPrefix, otherwise set origin with the ContainerEntityPrefix | ||
| if _, err := uuid.Parse(entityIDValue); err == nil { | ||
| k8sOrigin = kubelet.KubePodTaggerEntityPrefix + entityIDValue | ||
| } else { | ||
| origin = containers.ContainerEntityPrefix + entityIDValue | ||
| } |
There was a problem hiding this comment.
We should not assume that a UUID can only be a k8s pod UUID. I'm thinking it would be better if the dsd client added the container_id:// prefix when needed.
Then we can call this function , if the string is not an entity name we just add the kubelet.KubePodTaggerEntityPrefix prefix (default behaviour) otherwise we just set the origin without adding any prefix.
This ensures backward compatibility.
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
|
Hi, We are closing this PR as there was no recent activity on it. Feel free to reopen it if you still need the feature. |
5 participants
What does this PR do?
Currently, using the
dd.internal.entity_idStatsd tag to manually set a Dogstatsd metric's origin is only supported in Kubernetes. This change allows a user to set a metric's origin to a container ID.If the value of
dd.internal.entity_idis a UUID, assume it is a Kubernetes pod UID and setk8sOrigin. Otherwise, assume it is a container ID and setorigin.Additionally, the existing tests for
k8sOriginall used non-UUIDs, which does not conform to the real Kubernetes format for pod UIDs, defined here. This PR updates the tests to reflect the correct format.Motivation
We are currently running Datadog agents in ECS as sidecars on Fargate. In this setup, we can't use UDS (can't have a shared socket), so we have to use UDP to send metrics. Because we're using UDP, the metrics don't have an origin, so attaching auto-discovered tags to the incoming metrics does not work.
If we can manually set the origin in our metrics, then we can attach auto-discovered tags to the metrics.
Additional Notes
Currently, it is assumed that the value of
entityIDValueis a Kubernetes Pod UID, which is what the usage example in the Datadog docs indicates. However, the user could setDD_ENTITY_IDto whatever value they like and the entity ID would still bekubernetes_pod_uid://<not a pod UID>. I'm unsure if that use-case is intentional, but it does seems counter intuitive.The assumption that
DD_ENTITY_IDwill be a Pod UID as per the documentation is strictly enforced by this PR, as it validates that the incoming value is a UUID to determine if it is a Kubernetes origin or not.Also, this PR assumes that any non-UUID
dd.internal.entity_idshould be a container ID and used asorigin. This is also a large assumption, but there is no set format for container IDs, so I could not validate the value was in fact a container ID. I'd love suggestions otherwise.Describe how to test your changes
I did the following to test
dd.internal.entity_idis a container ID, not a UUIDk8sOriginto use UUIDs, to match the K8s Pod UID specI ran the test command as per the README
Checklist
changelog/no-changeloglabel has been applied.need-change/operatorandneed-change/helmlabels has been applied if applicable.team/..label has been applied, if known.Triagemilestone is set.Note: Adding GitHub labels is only possible for contributors with write access.