-
Notifications
You must be signed in to change notification settings - Fork 789
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
Rename @opentelemetry/sdk-metrics-base to @opentelemetry/sdk-metrics #3162
Rename @opentelemetry/sdk-metrics-base to @opentelemetry/sdk-metrics #3162
Conversation
Only changed references to the package, all the code still remains under "./experimental/packages/opentelemetry-sdk-metrics-base" moving the code will create a huge amount of files changes making it really hard to review, I can change the path of the files in different PR if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for your contribution!
@@ -228,7 +228,7 @@ Maintainers ([@open-telemetry/js-maintainers](https://github.com/orgs/open-telem | |||
| Package | Description | | |||
| ----------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | |||
| [@opentelemetry/sdk-trace-base][otel-tracing] | This module provides a full control over instrumentation and span creation. It doesn't load [`async_hooks`](https://nodejs.org/api/async_hooks.html) or any instrumentation by default. It is intended for use both on the server and in the browser. | | |||
| [@opentelemetry/sdk-metrics-base][otel-metrics] | This module provides instruments and meters for reporting of time series data. | | |||
| [@opentelemetry/sdk-metrics][otel-metrics] | This module provides instruments and meters for reporting of time series data. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link "otel-metrics" needs to be updated too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't update that one because I haven't moved the code yet, so the link will be broken, will update in following PR changing the actual metrics sdk code path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this PR not change the directory path?
edit: saw reasoning above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just a few minor comments. Thanks for working on this! 🙂
👍 for changing the path name in a follow up. 🙂
experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/OTLPMetricExporter.ts
Outdated
Show resolved
Hide resolved
Git and GitHub can distinguish file renames. As long as there are no significant changes in the renamed file, I believe there wouldn't be things more than a list of files being renamed. As far as I can tell, the only changes to be made to rename the directory Update: I would prefer to apply all the renames in this single PR to avoid confusion. |
@legendecas my main concern was that it was going to be harder to review and easier to miss something, people already review the actual package references so we should be good pushing new changes here if you think is better. |
yeah, I'd prefer to apply the renaming in this single PR to avoid confusion about the mismatch between the package name and the directory name. @hectorhdzg @open-telemetry/javascript-approvers what do you think? |
@dyladan @vmarchaud I changed the files path as well in this PR as @legendecas suggested, please take another look. |
I'm still fine with my approval |
same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Which problem is this PR solving?
Renaming @opentelemetry/sdk-metrics-base package to @opentelemetry/sdk-metrics
Fixes #3137
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: