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

Rename @opentelemetry/sdk-metrics-base to @opentelemetry/sdk-metrics #3162

Merged

Conversation

hectorhdzg
Copy link
Member

@hectorhdzg hectorhdzg commented Aug 12, 2022

Which problem is this PR solving?

Renaming @opentelemetry/sdk-metrics-base package to @opentelemetry/sdk-metrics

Fixes #3137

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

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

  • Unit tests

Checklist:

  • Followed the style guidelines of this project

@hectorhdzg hectorhdzg requested a review from a team August 12, 2022 00:24
@hectorhdzg
Copy link
Member Author

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.

@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #3162 (f706b5a) into main (3cca2ce) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3162      +/-   ##
==========================================
- Coverage   93.23%   93.21%   -0.02%     
==========================================
  Files         196      196              
  Lines        6414     6414              
  Branches     1350     1350              
==========================================
- Hits         5980     5979       -1     
- Misses        434      435       +1     
Impacted Files Coverage Δ
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 100.00% <ø> (ø)
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 44.82% <ø> (ø)
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <ø> (ø)
...ry-exporter-prometheus/src/PrometheusSerializer.ts 95.34% <ø> (ø)
...ntelemetry-sdk-metrics/src/InstrumentDescriptor.ts 100.00% <ø> (ø)
...kages/opentelemetry-sdk-metrics/src/Instruments.ts 94.73% <ø> (ø)
...al/packages/opentelemetry-sdk-metrics/src/Meter.ts 100.00% <ø> (ø)
...ges/opentelemetry-sdk-metrics/src/MeterProvider.ts 100.00% <ø> (ø)
.../opentelemetry-sdk-metrics/src/ObservableResult.ts 96.00% <ø> (ø)
...s/opentelemetry-sdk-metrics/src/aggregator/Drop.ts 100.00% <ø> (ø)
... and 46 more

Copy link
Member

@legendecas legendecas left a 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. |
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

@dyladan dyladan Aug 12, 2022

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

Copy link
Member

@pichlermarc pichlermarc left a 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/CHANGELOG.md Show resolved Hide resolved
@legendecas
Copy link
Member

legendecas commented Aug 15, 2022

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.

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 experimental/packages/opentelemetry-sdk-metrics-base are the links in the documentation. Code files are referencing it using the package name, instead of the directory name, and the package name has already been renamed in this PR. So I don't see why it's a problem to rename the directory too in this PR.

Update: I would prefer to apply all the renames in this single PR to avoid confusion.

@hectorhdzg
Copy link
Member Author

hectorhdzg commented Aug 15, 2022

@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.

@legendecas
Copy link
Member

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?

@hectorhdzg
Copy link
Member Author

hectorhdzg commented Aug 15, 2022

@dyladan @vmarchaud I changed the files path as well in this PR as @legendecas suggested, please take another look.

@dyladan
Copy link
Member

dyladan commented Aug 15, 2022

I'm still fine with my approval

@vmarchaud
Copy link
Member

same

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you!

@legendecas legendecas merged commit 6807def into open-telemetry:main Aug 16, 2022
@legendecas legendecas added this to the Metrics GA milestone Sep 7, 2022
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.

Rename @opentelemetry/sdk-metrics-base to @opentelemetry/sdk-metrics?
5 participants