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/dockerstats] Featuregate for new mdatagen implementation + semantic convention alignment #12743

Merged

Conversation

jamesmoessis
Copy link
Contributor

Description:
This PR:

  1. Allows using the new implementation of scrape (scrapeV2) via a feature gate
  2. Aligns the units with the system metrics semantic convention
  3. Enhances the tests to test for correct units and descriptions (for the new implementation only)

Link to tracking Issue: #9794

Testing:
Units are changed in expected_metrics.json files, as well as descriptions added. The legacy scrape implementation is now not tested in comparison to the new one, since they are have different units and descriptions. They have been proven equivalent by the last PR, and now they will start to diverge as we make enhancements to the new implementation.

Documentation: Documented the feature gate in the README.

@jamesmoessis jamesmoessis requested review from a team and dmitryax July 27, 2022 01:06
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

Just a few comments on it but it is straight forward enough.

I do need to revisit what is the recommended time to give users for breaking changes.

receiver/dockerstatsreceiver/factory.go Show resolved Hide resolved
receiver/dockerstatsreceiver/factory.go Outdated Show resolved Hide resolved
receiver/dockerstatsreceiver/README.md Show resolved Hide resolved
receiver/dockerstatsreceiver/README.md Outdated Show resolved Hide resolved
@jamesmoessis
Copy link
Contributor Author

Looks like there's an unrelated windows test failing the build

@jamesmoessis
Copy link
Contributor Author

@dmitryax is there anything left to do to get this PR merged? Perhaps a codeowner review from @rmfitzpatrick might help?

@dmitryax dmitryax merged commit 651712b into open-telemetry:main Aug 1, 2022
@dmitryax
Copy link
Member

dmitryax commented Aug 1, 2022

@jamesmoessis LGTM. But it's better to submit separate PRs for unrelated even small changes

@jamesmoessis jamesmoessis deleted the dockerstats-featuregate-new-impl branch August 2, 2022 00:43
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.

4 participants