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

[OpenTelemetry Collector] Update example with new configuration #13787

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Apr 22, 2022

What does this PR do?

Change tags setting to reflect the new host_metadata section from open-telemetry/opentelemetry-collector-contrib/pull/9100, available from v0.49.0

Motivation

As documented in open-telemetry/opentelemetry-collector-contrib/issues/9099, the tags setting is deprecated and will eventually be removed in favor of the host_metadata::tags section.

Preview

https://docs-staging.datadoghq.com/<BRANCH_NAME>/

Additional Notes


Reviewer checklist

  • Review the changed files.
  • Review the URLs listed in the Preview section.
  • Check images for PII
  • Review any mentions of "Contact Datadog support" for internal support documentation.

@mx-psi mx-psi requested a review from a team April 22, 2022 11:00
@mx-psi mx-psi requested a review from a team as a code owner April 22, 2022 11:00
@github-actions github-actions bot added the tracing Content changed in the tracing folder label Apr 22, 2022
- example:tag
host_metadata:
tags:
- example:tag

api:
key: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that you change that and it is not a big deal at all but IMHO aaaaa... looks bit strange. Arguably xxxxxx... or XXXX... more traditional substitution. And I think [xxxxx...] or <xxx...> have a better implied intentions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa used for API key at Datadog more than once, and I personally prefer it to xxxx or XXXX.
"a" is the first letter of the alphabet and it sounds like someone saying "aaaaaaah", like "aaaaah we're missing the API key".
But you can't pronounce "XXXXX" :)
I don't particularly agree that "XXXX" has better implied intentions.

Copy link
Member Author

@mx-psi mx-psi Apr 22, 2022

Choose a reason for hiding this comment

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

👍 that's a good point. I think the right choice here is to put as an example:

api:
  key: ${DD_API_KEY}

and pass the API key as an environment variable, which is the recommended setup for sensitive fields.

This needs to change in a few places though and I think I want to wait a bit until we have made a change on environment variable resolution, but I will make a note, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@julien-lebot

I've seen aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa used for API key at Datadog more than once, and I personally prefer it to xxxx or XXXX.
"a" is the first letter of the alphabet and it sounds like someone saying "aaaaaaah", like "aaaaah we're missing the API key".
But you can't pronounce "XXXXX" :)
I don't particularly agree that "XXXX" has better implied intentions.

That teaches me that what I am very biased even about most obvious (for me) thing :)

Copy link
Contributor

@alai97 alai97 left a comment

Choose a reason for hiding this comment

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

Looks good for docs!

@mx-psi
Copy link
Member Author

mx-psi commented Apr 22, 2022

@alai97 any reason why the build preview job is not running?

@alai97 alai97 merged commit f31a484 into master Apr 22, 2022
@alai97 alai97 deleted the mx-psi-patch-1 branch April 22, 2022 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracing Content changed in the tracing folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants