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.

Sign up for GitHub

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

Jump to bottom

[receiver/hostmetrics] remove direction attribute #11820

Merged
merged 5 commits into from
Jun 30, 2022
Merged

[receiver/hostmetrics] remove direction attribute #11820

merged 5 commits into from
Jun 30, 2022

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 review from a team and dmitryax as code owners 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
[receiver/hostmetrics/networkscraper] remove direction
e9f71d1 
The following change adds the direction to the metric name, removing the `direction` attribute.

Fixes #11817
update changelog
4b26ad3
put direction change behind feature gate
c9e63c4
@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

dmitryax
dmitryax reviewed Jun 29, 2022
View reviewed changes
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

receiver/hostmetricsreceiver/internal/scraper/networkscraper/metadata.yaml Show resolved Hide resolved
@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
atoulme pushed a commit to atoulme/opentelemetry-collector-contrib that referenced this pull request Jul 16, 2022
@codeboten @atoulme
[receiver/hostmetrics] remove direction attribute (open-telemetry#11820)
8b96bb0 
* [receiver/hostmetrics/networkscraper] remove direction

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

Fixes open-telemetry#11817

* update changelog

* put direction change behind feature gate

* add comment
@sweetpotato0
Copy link

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

@dmitryax
Copy link
Member

dmitryax commented Nov 9, 2022 •
edited
Loading

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

@rubenruizdegauna rubenruizdegauna mentioned this pull request Dec 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitryax dmitryax dmitryax approved these changes

Assignees

@jpkrohling jpkrohling

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[receiver/hostmetrics/networkscraper]: remove direction attribute
4 participants
@codeboten @dmitryax @sweetpotato0 @jpkrohling