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

Bump opentelemetry collector 0.85 #5290

Merged
merged 59 commits into from
Sep 29, 2023
Merged

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Sep 25, 2023

PR Description

Bump opentelemetry-collector and opentelemetry-collector-contrib from v0.80 to v0.85.
The agent will use the release version from the Grafana fork where I had to adjust and apply the previous patch made my @ptodev

Which issue(s) this PR fixes

Notes to the Reviewer

I followed the process made by @ptodev described in https://github.com/grafana/agent/pull/4343/files

I made all the changes concerning our components except for:

  • otelcol.receiver.prometheus
    The implementations of these two components differ from the opentelemetry ones and can be updated later.

Some changes

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@wildum wildum requested review from a team and clayton-cornell as code owners September 25, 2023 13:33
@tpaschalis
Copy link
Member

I made all the changes concerning our components except for: otelcol.exporter.prometheus, otelcol.receiver.prometheus.
The implementations of these two components differ from the opentelemetry ones and can be updated later.

What do you expect these changes to be? Could we at least leave a TODO comment or open an issue to keep track of this?

@wildum
Copy link
Contributor Author

wildum commented Sep 27, 2023

I made all the changes concerning our components except for: otelcol.exporter.prometheus, otelcol.receiver.prometheus.
The implementations of these two components differ from the opentelemetry ones and can be updated later.

What do you expect these changes to be? Could we at least leave a TODO comment or open an issue to keep track of this?

I have updated the description of the PR because after closer inspection this concerns only the prometheus receiver. We copied the internal folder and some of these files changed through the releases (there are several changes like this one for example: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/23448/files#diff-a71211e5426c3d12d9c3c0b5991e4b284568b76310438f884a37ce20655327f4)
I opened an issue to take care of this update: #5313

@wildum wildum requested a review from ptodev September 27, 2023 09:22
Copy link
Contributor

@ptodev ptodev left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! I added a few comments. Also, please make sure the static mode docs are updated as per this comment.

I think we should be ok to merge after those are resolved 🚢

component/otelcol/auth/oauth2/oauth2.go Outdated Show resolved Hide resolved
@@ -14,7 +14,7 @@ import (
"go.opentelemetry.io/otel/exporters/otlp/otlptrace"
"go.opentelemetry.io/otel/sdk/resource"
tracesdk "go.opentelemetry.io/otel/sdk/trace"
semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

we seem to import different versions depending on the files

I think pkg/flow/tracing/tracing.go is the only place where we import go.opentelemetry.io/otel/semconv/?

I agree that this is confusing. I did some digging just now, and this is what I think is happening:

  • The opentelemetry-specification repo is the one which publishes a spec, and this is what the "v1.17.0" was based on.
  • Then the Otel team decided to move the semantic conventions to the semantic-conventions repo. v1.21.0 is the first semconv from that repo.
  • The opentelemetry-go repo is the Go API which implements the "opentelemetry-specification" and "semantic-conventions" for Go.

As per go.mod, we upgrade opentelemetry-go from v1.16.0 to v1.17.0. For each version of opentelemetry-go, there are implementations of the various semantic conventions.

So what I think needs to be done is to eyeball these changelogs for breaking changes:

@@ -78,6 +78,7 @@ Name | Type | Description | Default | Required
`wait_for_ready` | `boolean` | Waits for gRPC connection to be in the `READY` state before sending data. | `false` | no
`headers` | `map(string)` | Additional headers to send with the request. | `{}` | no
`balancer_name` | `string` | Which gRPC client-side load balancer to use for requests. | `pick_first` | no
`authority` | `string` | Overrides the default `:authority` header in gRPC requests from the gRPC client. | | no
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to document it in a similar way to how balancer_name is documented - check out this file for an example:
agent/docs/sources/shared/flow/reference/components/otelcol-grpc-balancer-name.md

@wildum wildum requested a review from ptodev September 28, 2023 15:45
@wildum wildum requested a review from ptodev September 29, 2023 09:48
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this file is being change on this branch?

@ptodev ptodev merged commit 3b8fc4a into main Sep 29, 2023
@ptodev ptodev deleted the bump-opentelemetry-collector-0.85 branch September 29, 2023 11:15
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants