Skip to content

[OTel] Remove explicit tags for Collector image #15586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 24, 2022
Merged

[OTel] Remove explicit tags for Collector image #15586

merged 1 commit into from
Oct 24, 2022

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Oct 14, 2022

What does this PR do?

Remove requirement for using a pinned version of the Collector. In general, we want users to use the latest version.

Motivation

open-telemetry/opentelemetry-collector-releases/issues/73 was fixed.

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 October 14, 2022 08:55
@mx-psi mx-psi requested a review from a team as a code owner October 14, 2022 08:55
@github-actions github-actions bot added the tracing Content changed in the tracing folder label Oct 14, 2022
@github-actions
Copy link
Contributor

Preview links (active after the build_preview check completes)

Modified Files

gbbr
gbbr approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Nice!

content/en/tracing/trace_collection/open_standards/otel_collector_datadog_exporter.md
@@ -313,7 +308,7 @@ To use the OpenTelemetry Operator:
spec:
mode: daemonset
hostNetwork: true
image: otel/opentelemetry-collector-contrib:0.59.0
image: otel/opentelemetry-collector-contrib
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this then defaults to latest, based on the description of the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup!

dineshg13
dineshg13 requested changes Oct 14, 2022
View reviewed changes
Copy link
Member

@dineshg13 dineshg13 left a comment

Choose a reason for hiding this comment

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

Using latest tag in k8s environment is dangerous. It depends on https://kubernetes.io/docs/concepts/containers/images/#image-pull-policy , whether the collector is using latest image or not. And also it can happen that multiple nodes will have multiple versions running.

@mx-psi
Copy link
Member Author

mx-psi commented Oct 17, 2022

Using latest tag in k8s environment is dangerous. It depends on https://kubernetes.io/docs/concepts/containers/images/#image-pull-policy , whether the collector is using latest image or not. And also it can happen that multiple nodes will have multiple versions running.

Telling people to use latest is consistent with how we tell people to use the Agent in Docker (the 7 tag gets autoupdated to the latest 7.x version), and the Agent operator, I am just trying to follow the established convention. If people want to pin the version they are free to do it, we are not making it harder to do so with this change. Do you think the situation here is different from that of the Agent?

urseberry
urseberry approved these changes Oct 18, 2022
View reviewed changes
Copy link
Contributor

@urseberry urseberry left a comment

Choose a reason for hiding this comment

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

Ping #documentation on Slack if you need this merged. I'm not merging now, since I see there is still some discussion going on.

dineshg13
dineshg13 approved these changes Oct 24, 2022
View reviewed changes
Copy link
Member

@dineshg13 dineshg13 left a comment

Choose a reason for hiding this comment

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

As discussed offline, we should be ok following the convention established by agent.

@urseberry urseberry merged commit 844d8fc into master Oct 24, 2022
@urseberry urseberry deleted the mx-psi/latest-contrib branch October 24, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dineshg13 dineshg13 dineshg13 approved these changes

@gbbr gbbr gbbr approved these changes

@urseberry urseberry urseberry approved these changes

Assignees
No one assigned
Labels
tracing Content changed in the tracing folder
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mx-psi @dineshg13 @gbbr @urseberry