opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

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

Open hectorhdzg opened this issue 2 years ago • 2 comments

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)
  • [ ] This change requires a documentation update

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 avatar Aug 12 '22 00:08 hectorhdzg

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.

hectorhdzg avatar Aug 12 '22 00:08 hectorhdzg

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

codecov[bot] avatar Aug 12 '22 00:08 codecov[bot]

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.

legendecas avatar Aug 15 '22 02:08 legendecas

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

hectorhdzg avatar Aug 15 '22 17:08 hectorhdzg

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?

legendecas avatar Aug 15 '22 17:08 legendecas

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

hectorhdzg avatar Aug 15 '22 17:08 hectorhdzg

I'm still fine with my approval

dyladan avatar Aug 15 '22 17:08 dyladan

same

vmarchaud avatar Aug 15 '22 18:08 vmarchaud