Skip to content
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

feat(origin detection): send both container ID and Entity ID #828

Merged
merged 1 commit into from
May 7, 2024

Conversation

wdhif
Copy link
Member

@wdhif wdhif commented Apr 15, 2024

What does this PR do?

We would like customers to be able to retrieve container tags even when DD_ENTITY_ID is set. Currently the library does not send the container ID if the Entity ID is set.

For those who do not want container tags, several alternatives are possible:

  • Send the tag dd.internal.card=none.
  • Inject DD_ORIGIN_DETECTION_ENABLED=false to the application pod.

Description of the Change

Remove checks for DD_ENTITY_ID when resolving container ID.

Possible Drawbacks

Verification Process

To make sure that this is working as intended, create a new pod with the following manifest:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: dummy-dogstatsd-udp-app
spec:
  replicas: 1
  selector:
    matchLabels:
      app: dummy-dogstatsd-udp-app
  template:
    metadata:
      labels:
        app: dummy-dogstatsd-udp-app
    spec:
      containers:
      - name: dummy-dogstatsd-udp-app
        image: wdhifdatadog/dummy_dogstatsd_udp
        imagePullPolicy: Always
        env:
        - name: DD_AGENT_HOST
          valueFrom:
            fieldRef:
              fieldPath: status.hostIP
        - name: DD_ENTITY_ID
          valueFrom:
            fieldRef:
              fieldPath: metadata.uid

You should still be able to get the following metrics with container tags such as container_id:

  • dummy_dsd_udp_python.increment
  • dummy_dsd_udp_python.decrement

image

We can see that, with an agent cardinality set as high:

  • With the old behaviour, we don't get container tags.
  • With the old behaviour and dd.internal.card:low, we don't get container tags (only pod tags).
  • With the new behaviour, we get container tags.
  • With the new behaviour and dd.internal.card:low, we don't get container tags (only pod tags).

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@wdhif wdhif requested review from a team as code owners April 15, 2024 12:02
@wdhif wdhif marked this pull request as draft April 15, 2024 12:03
@wdhif wdhif added resource/dogstatsd changelog/Changed Changed features results into a major version bump labels Apr 15, 2024
@wdhif wdhif marked this pull request as ready for review April 15, 2024 12:33
@wdhif
Copy link
Member Author

wdhif commented Apr 15, 2024

Links to DataDog/datadog-go#300

@wdhif wdhif marked this pull request as draft April 15, 2024 12:59
@wdhif wdhif force-pushed the CONTINT-3897/wassim.dhif/both-entity-id-container-id branch 4 times, most recently from fd9db81 to 87abc5a Compare April 15, 2024 13:23
Signed-off-by: Wassim DHIF <wassim.dhif@datadoghq.com>
@wdhif wdhif force-pushed the CONTINT-3897/wassim.dhif/both-entity-id-container-id branch from 87abc5a to f171e20 Compare April 15, 2024 13:23
@wdhif wdhif marked this pull request as ready for review April 15, 2024 13:26
@wdhif
Copy link
Member Author

wdhif commented Apr 15, 2024

I've added a unit test showing both Entity ID and container ID in the same metric.

@wdhif wdhif self-assigned this Apr 30, 2024
@wdhif
Copy link
Member Author

wdhif commented May 7, 2024

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 7, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

@dd-devflow
Copy link

dd-devflow bot commented May 7, 2024

🚨 MergeQueue

Gitlab pipeline didn't start on its own and we were unable to create it... Please retry.

If you need support, contact us on Slack #devflow with those details!

@wdhif
Copy link
Member Author

wdhif commented May 7, 2024

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 7, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

@dd-devflow
Copy link

dd-devflow bot commented May 7, 2024

🚨 MergeQueue

Gitlab pipeline didn't start on its own and we were unable to create it... Please retry.

If you need support, contact us on Slack #devflow with those details!

@wdhif wdhif merged commit b05583e into master May 7, 2024
33 of 34 checks passed
@wdhif wdhif deleted the CONTINT-3897/wassim.dhif/both-entity-id-container-id branch May 7, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Changed Changed features results into a major version bump mergequeue-status: error resource/dogstatsd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants