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

Bumped OpenTelemetry Collector to v0.13.0 #101

Merged
merged 5 commits into from
Oct 22, 2020

Conversation

dengliming
Copy link
Member

closes #93

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this one!

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. I'll check #100 before releasing this, in case it's a real bug that needs fixing.

@jpkrohling
Copy link
Member

@mergify rebase

@mergify
Copy link

mergify bot commented Oct 22, 2020

Command rebase: success

Branch has been successfully rebased

Hey, I reacted but my real name is @Mergifyio

@jpkrohling
Copy link
Member

Checking the release notes, we need to handle the change open-telemetry/opentelemetry-collector#1891. Either we need to replace the image with the contrib one (v0.13.1, which includes this processor), or remove the processor. In any case, we need to add a message to the status object.

@objectiser, what do you think would be more appropriate?

@objectiser
Copy link
Contributor

@jpkrohling when you say "remove the processor", remove it from where?

@jpkrohling
Copy link
Member

From the CR's configuration.

This processor (groupbytrace) has been removed from the core distribution and added to the contrib on v0.13.1. CRs that are configured using this processor will fail if we don't change the configuration during the upgrade, so, we should either automatically remove it from the CR, or change the CR's image to use contrib.

@objectiser
Copy link
Contributor

objectiser commented Oct 22, 2020

@jpkrohling As this is pre-1.0, and components are still being shuffled between core and contrib, not sure the operator should take care of all these situations yet? Maybe just something to mention in the CHANGELOG?

@jpkrohling
Copy link
Member

The code itself shouldn't be that hard once we decide what's the best approach, and this might help guide future decisions. I'd be OK in just adding something to the changelog as well :-)

CHANGELOG.md Outdated Show resolved Hide resolved
@objectiser
Copy link
Contributor

@jpkrohling yeah we need to have a policy - although one of the issues is that users can specify their own custom image (whether contrib or something else) and the operator will only be able to deal with what it knows about - just depends whether that is just core, or should it also cover contrib components?

@jpkrohling
Copy link
Member

What I had in mind is to add a "compatibility" flag to the new CR (#77 (comment)), stating how we should treat the CR for upgrades.

@jpkrohling
Copy link
Member

Given that we don't currently know what's the distribution people are using, it's probably safer to ignore this upgrade for this release.

@dengliming, would you please then add a mention in the changelog that people have to manually deal with this move?

@dengliming
Copy link
Member Author

The groupbytrace processor already be moved to the contrib repository. Either we need to replace the image with the contrib one (v0.13.1, which includes this processor), or remove the processor.

what about this?

@jpkrohling
Copy link
Member

Sounds good, just a small change:

The `groupbytrace` processor was moved to the contrib repository, requiring a manual intervention in case this processor is being used: either replace the image with the contrib one (v0.13.1, which includes this processor), or remove the processor.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again for your help, @dengliming! The new release is coming right away.

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.

Update to collector v0.13.0
3 participants