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

Update protocol defined for internal collection #6279

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mjuarez
Copy link
Contributor

@mjuarez mjuarez commented Feb 10, 2025

Per open-telemetry/opentelemetry-collector#12332, a breaking change for the protocol used was introduced

@mjuarez mjuarez requested a review from a team as a code owner February 10, 2025 22:42
@opentelemetrybot opentelemetrybot requested review from a team and jpkrohling and removed request for a team February 10, 2025 22:43
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

LGTM. Awaiting final approval of @open-telemetry/collector-approvers.

@chalin chalin added the sig-approval-missing Co-owning SIG didn't provide an approval label Feb 10, 2025
@tiffany76
Copy link
Contributor

Thanks for staying on top of the changes, @mjuarez! I see that the changelog update isn't quite merged yet, so let's ask the @open-telemetry/collector-approvers to take a look.

One additional question. Do we need to add a comment about versions? Something like, "if you're using Collector v0.118.0 or older, grpc/protobuf or grpc can be used. From v0.119.0, grpc/protobuf is no longer valid." I don't know if this is useful or relevant information to users but figured I'd ask.

@mjuarez
Copy link
Contributor Author

mjuarez commented Feb 11, 2025

I know the changelog update isn't in, but had a peer on my team reference the existing doc while using the latest published version (0.119.0) and it was broken for them. So anyone new to otel-collector wouldn't be set up for success.

I'm unsure about distinguishing config options by version. Has that been used elsewhere in the docs I can use as an example? Happy to update.

@tiffany76 tiffany76 removed the sig-approval-missing Co-owning SIG didn't provide an approval label Feb 11, 2025
@tiffany76
Copy link
Contributor

I think we can skip the versioning comment, since as you point out @mjuarez, this is a configuration change.

With apologies for delaying this PR a bit longer.... As I was looking through the internal telemetry page, I saw that grpc/protobuf is used again further down, on line 172. Does it need to be updated there as well, @mx-psi?

@mx-psi
Copy link
Member

mx-psi commented Feb 12, 2025

Ah, good catch, yes! This needs to be removed as well

@tiffany76
Copy link
Contributor

@mjuarez, if you could remove the /protobuf from line 172 as well, I think we can call this one done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants