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

[receiver/hostmetrics] remove direction attribute #11820

Merged

Conversation

codeboten
Copy link
Contributor

The following change adds the direction to the metric name, removing the direction attribute.

Fixes #11817

@codeboten codeboten force-pushed the codeboten/direction-networkscraper branch from c11169d to aef3030 Compare June 29, 2022 18:38
@codeboten codeboten marked this pull request as ready for review June 29, 2022 18:39
@codeboten codeboten requested a review from a team June 29, 2022 18:39
@codeboten codeboten requested a review from dmitryax as a code owner June 29, 2022 18:39
@dmitryax
Copy link
Member

I think this receiver is stable enough to avoid making such breaking changes. I believe many distributions depend on it.

Can we make it through a feature gate (or enabling/disabling metrics) spanned across several versions in the following way?
0.55.0: keep reporting the old metrics with a warning that they will be replaced and provide a way to switch to the new metrics through a feature gate or disabling/enabling metrics.
0.57.0: switch to the new metrics with an option to bring the old metrics back.
0.59.0: remove old metrics.

@codeboten
Copy link
Contributor Author

reporting the old metrics with a warning

Would this warning show up at collector start?

@dmitryax
Copy link
Member

Would this warning show up at collector start?

Yes, we've done this many times for such breaking changes

Alex Boten added 3 commits June 29, 2022 14:17
The following change adds the direction to the metric name, removing the `direction` attribute.

Fixes #11817
@codeboten codeboten force-pushed the codeboten/direction-networkscraper branch from aef3030 to c9e63c4 Compare June 29, 2022 21:17
@codeboten
Copy link
Contributor Author

Would this warning show up at collector start?

Yes, we've done this many times for such breaking changes

@dmitryax it's behind a feature gate now, please take a look

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Overal LGTM

@codeboten codeboten merged commit 10f4bd0 into open-telemetry:main Jun 30, 2022
@codeboten codeboten deleted the codeboten/direction-networkscraper branch June 30, 2022 19:44
@sweetpotato0
Copy link

i have a quetion: why set the some metric to Deprecated, such as system.network.droppedsystem.network.errors

@dmitryax
Copy link
Member

dmitryax commented Nov 9, 2022

@sweetpotato0 thanks for pointing to this. The deprecated labels should've been removed some time ago. I submitted #16227 to fix it

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.

[receiver/hostmetrics/networkscraper]: remove direction attribute
4 participants