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

chore: Replace dependency on unexported otelcol functions #2148

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dehaansa
Copy link
Contributor

@dehaansa dehaansa commented Nov 22, 2024

PR Description

Inspired by #1023 - replacing the call to an unexported function to utilizing the intended functionality from otelcol.

  • Tests updated
  • Config converters updated

@dehaansa dehaansa requested a review from a team as a code owner November 22, 2024 21:48
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.

Amazing, thank you! I didn't realise that it's possible to do this 😆 I thought the public OTel functions only allow generating the config from a file.

internal/static/traces/config_test.go Outdated Show resolved Hide resolved
@dehaansa dehaansa requested a review from ptodev November 26, 2024 16:40
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! Would you like to also do the same change to the Agent?
We won't be able to rid of the fork entirely, but it'll help - there will be less things to modify on the fork when we upgrade it.

@wildum
Copy link
Contributor

wildum commented Nov 28, 2024

@ptodev I don't believe that we are planning on updating the Otel version for the Agent

@ptodev
Copy link
Contributor

ptodev commented Nov 28, 2024

I suppose so, but we might have to, you never know :)

@ptodev
Copy link
Contributor

ptodev commented Nov 28, 2024

I don't insist on the change being done on the Agent. I'll leave it up to @dehaansa to decide if it's worth his effort. Maybe it's too much risk for too little benefit.

@wildum
Copy link
Contributor

wildum commented Nov 28, 2024

We already use a custom version of the fork for the agent because of some CVEs (see https://github.com/grafana/agent/blob/main/go.mod#L777) so to get rid of the fork we would either need to copy some more code or we would need to upgrade the Otel version at the same time to get the CVEs fix.
I personally don't think that's worth the effort

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

Successfully merging this pull request may close these issues.

3 participants