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

Move old metrics implementation to different directory, and rename targets to *_deprecated #1053

Merged
merged 10 commits into from
Nov 5, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Nov 4, 2021

Changes

As the effort required to update our existing metrics SDK implementation as per new specs is huge, it's easier to re-write it from the ground-up ( #1033 ). This means we could

  • Either remove the old implementation, which breaks any customers who would be using it, and leave them with no alternative till the new implementation is ready.
  • Or let both old and new implementation exists together for some time till new implementation is ready. The old implementation exists under the feature flag WITH_METRICS_PREVIEW. And the new implementation would be active/build only if the feature flag is disabled.

We discussed both approaches in the community meeting and agreed on second. This PR makes necessary changes for it, basically

  • Move the old implementation to */_metrics/* directory, and new development continue under */metrics/*.
  • Rename cmake and bazel target for old implementation to opentelemetry_metrics_deprecated, prometheus_exporter_deprecated

ps. - We can also use this PR on discussing both approaches if required. The first approach does provide cleaner repo and easy maintainability.

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team November 4, 2021 19:28
@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #1053 (8aa27a4) into main (14ec785) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1053   +/-   ##
=======================================
  Coverage   94.85%   94.85%           
=======================================
  Files         151      151           
  Lines        5971     5971           
=======================================
  Hits         5663     5663           
  Misses        308      308           

@ThomsonTan ThomsonTan merged commit e58524c into open-telemetry:main Nov 5, 2021
@lalitb lalitb mentioned this pull request Nov 5, 2021
3 tasks
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.

3 participants