Upgrade OTel collector to v0.27.0 - #641
Conversation
Fork uses cmd's context instead of background which is necessary for our custom processor and exporters.
yvrhdn
left a comment
yvrhdn
left a comment
There was a problem hiding this comment.
Some small comments, didn't get through all the changes yet.
mapno
changed the title
| Factories: factories, | ||
| BuildInfo: buildInfo, | ||
| ParserProvider: &cfg, | ||
| LoggingOptions: []zap.Option{zap.Development()}, |
There was a problem hiding this comment.
Maybe we want to disable the app's own logging. I'm not sure with this one
rfratto
left a comment
rfratto
left a comment
There was a problem hiding this comment.
I'm going to leave the real review of this to people more familiar with the collector, but just a small nit about using personal orgs
| // loadbalancingexporter uses non-fixed version of batchpersignal which fetches latest and causes problems | ||
| replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal v0.27.0 | ||
|
|
||
| replace go.opentelemetry.io/collector => github.com/mapno/opentelemetry-collector v0.24.1-0.20210610132247-eb06509beaa2 |
There was a problem hiding this comment.
Can you move this to the grafana org? IMO we should have as few personal forks as possible. (I'm the worst offender of this and need to remove the rfratto instances here)
There was a problem hiding this comment.
What does v0.24.1-... refer to? Is it this commit: mapno/opentelemetry-collector@eb06509 Those Go version always confuse me.
Are you planning to PR these changes? If so, we can link the PR as comment, makes it easier to track when we can remove the fork.
There was a problem hiding this comment.
Any particular reason we're not vendoring otelcol directly?
There was a problem hiding this comment.
What does v0.24.1-... refer to? Is it this commit: mapno/opentelemetry-collector@eb06509 Those Go version always confuse me.
Correct. It's referencing that commit. I also find go modules confusing, but the reason for that version is that it's referencing an unreleased commit, thus it's using a pseudo-version. If I'm not mistaken, it's using the 3rd form for pseudo-versions, but I don't know why it's v0.24.x and not v0.27.x.
Are you planning to PR these changes? If so, we can link the PR as comment, makes it easier to track when we can remove the fork.
Any particular reason we're not vendoring otelcol directly?
Yes, that's the plan. I've just opened the PR to the collector. Initially I used a fork to get things moving fast, so we could integrate rapidly the new sampling policies we're working on. It'll probably won't be necessary to use my fork or Grafana's fork if it gets merged soon.
Can you move this to the grafana org?
I'll wait a bit to get a response from the upstream. If it doesn't get merged we can create the org fork and make it official.
There was a problem hiding this comment.
:woohoo: merged! open-telemetry/opentelemetry-collector#3418
yvrhdn
left a comment
yvrhdn
left a comment
There was a problem hiding this comment.
Left some remarks, but overall this seems good. Nice work Mario! 💪🏻
Disclaimer: I'm not familiar with the collector and how it's supposed to work, so I might be missing stuff 😓
| // loadbalancingexporter uses non-fixed version of batchpersignal which fetches latest and causes problems | ||
| replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal => github.com/open-telemetry/opentelemetry-collector-contrib/pkg/batchpersignal v0.27.0 | ||
|
|
||
| replace go.opentelemetry.io/collector => github.com/mapno/opentelemetry-collector v0.24.1-0.20210610132247-eb06509beaa2 |
There was a problem hiding this comment.
What does v0.24.1-... refer to? Is it this commit: mapno/opentelemetry-collector@eb06509 Those Go version always confuse me.
Are you planning to PR these changes? If so, we can link the PR as comment, makes it easier to track when we can remove the fork.
yvrhdn
left a comment
yvrhdn
left a comment
There was a problem hiding this comment.
We just have to decide what to do with the fork, but other than that LGTM 👍
|
Bumping this one more time to |
|
Closed in favor of #700 |
github-actions
Bot
added
the
frozen-due-to-age
4 participants
PR Description
Upgrades OTel collector from
v0.21.0tov0.27.0. The builder pkg is made internal and now we need to use theservice.Applicationinterface from two layers above.Which issue(s) this PR fixes
None.
Notes to the Reviewer
It's using a personal fork to propagate the context to the builders, which is needed for our custom processors and exporters.
PR Checklist