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

Add Datadog GlobalTags support #9266

Merged
merged 5 commits into from Sep 12, 2022

Conversation

sdelicata
Copy link
Contributor

What does this PR do?

This PR adds a new GlobalTags option for Datadog tracing.
The previous GlobalTag option has been deprecated.

Motivation

More

  • [ ] Added/updated tests
  • [ ] Added/updated documentation

Additional Notes

@kevinpollet kevinpollet added this to To review in v2 via automation Aug 18, 2022
@kevinpollet kevinpollet added this to the next milestone Aug 18, 2022
@sdelicata sdelicata force-pushed the feat_datadog_globaltags branch 2 times, most recently from 83bc159 to 513171f Compare August 22, 2022 09:07
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

LGTM 👌

Comment on lines 49 to 62
if c.GlobalTag != "" {
log.WithoutContext().Warn(`Datadog: option "globalTag" is deprecated, please use "globalTags" instead.`)

value := ""
if len(tag) == 2 {
value = tag[1]
key, value, _ := strings.Cut(c.GlobalTag, ":")

// Don't override a tag already defined with the new option.
if _, ok := c.GlobalTags[key]; !ok {
if c.GlobalTags == nil {
c.GlobalTags = make(map[string]string)
}

c.GlobalTags[key] = value
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

imho, it would have been better to make them mutually exclusive as soon as possible in the flow. It is highly unlikely the user purposefully uses both options at the same time, so we would do them a favor by detecting it, telling them to clean up their conf, and stopping.
But whatever, the option is going to disappear soon anyway.

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

2 minors suggestions

docs/content/observability/tracing/datadog.md Outdated Show resolved Hide resolved
docs/content/observability/tracing/datadog.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏

@traefiker traefiker merged commit 10528c9 into traefik:master Sep 12, 2022
v2 automation moved this from To review to Done Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
v2
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants