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

[exporter/datadog] Compatibility with ECS Fargate semantic conventions #6670

Merged
merged 6 commits into from
Dec 21, 2021

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Dec 9, 2021

Description:

This adds compatibility with AWS ECS Fargate OpenTelemetry semantic conventions by

Previously, we already added task_arn automatically to tags/metadata; this is kept.

Users of Fargate versions that don't have TMDE v4 or later, need to add the aws.ecs.launchtype semantic convention manually to payloads, e.g. by means of the resource attribute:

processors:
  resource:
    attributes:
    - key: aws.ecs.launchtype
      value: fargate
      action: upsert

Link to tracking Issue: n/a, customer request

Testing: Added unit tests, tested end to end by mocking:

Resource processor configuration for mocking AWS ECS Fargate
processors:
  resource:
    attributes:
    - key: aws.ecs.launchtype
      value: fargate
      action: upsert
    - key: aws.ecs.task.arn
      value: "example-task-arn"
      action: upsert

…y#5967)"

This reverts commit 247420a.

We can't always add the current hostname *and* not expose the hostname in Fargate.
…unning metrics

We do this because of two reasons:
1. we don't want to expose the Fargate hostname and
2. users care about identifying where their Collector is running (in this case which task).
@boostchicken
Copy link
Member

Just a note, I have a absolutely had use cases in large Fargate clusters where one or two hosts went bad and the only way we caught it in DD was with hostname / ip based metrics we were gathering. This was a cluster of a few thousand tasks so a full re-deployment was out of the question.

@jpkrohling jpkrohling changed the title [exporter/datadogexporter] Compatibility with ECS Fargate semantic conventions [exporter/datadog] Compatibility with ECS Fargate semantic conventions Dec 16, 2021
@mx-psi
Copy link
Member Author

mx-psi commented Dec 20, 2021

I have a absolutely had use cases in large Fargate clusters where one or two hosts went bad and the only way we caught it in DD was with hostname / ip based metrics we were gathering

It's useful to know this, thanks! Here I am trying to do a reduced version of what the Datadog Agent does when setting the ECS_FARGATE environment variable, where the hostname is removed and provided info is task ARN based instead.

As it happens with the Datadog Agent, you can still get the hostname if you want: only if you use the resourcedetection processor's ecs detector will we remove the hostname. I think this will enable us to support both users that want the hostnames and users who don't (but if you think this won't work for some reason please let me know :)) ).

Copy link
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment but LGTM overall

exporter/datadogexporter/internal/metrics/consumer.go Outdated Show resolved Hide resolved
@mx-psi mx-psi marked this pull request as ready for review December 21, 2021 10:00
@mx-psi mx-psi requested review from a team, tigrannajaryan and KSerrania December 21, 2021 10:00
@mx-psi
Copy link
Member Author

mx-psi commented Dec 21, 2021

@jpkrohling this has been approved by a CODEOWNER. If you find the time today, could you review and, if it looks good, merge it? Thanks!

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just wanted to double-check a specific conditional.

exporter/datadogexporter/translate_traces.go Show resolved Hide resolved
@jpkrohling jpkrohling merged commit 290975f into open-telemetry:main Dec 21, 2021
@mx-psi mx-psi deleted the mx-psi/ecs-fargate-support branch December 21, 2021 15:15
povilasv referenced this pull request in coralogix/opentelemetry-collector-contrib Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants