Skip to content

Conversation

@trentm
Copy link
Contributor

@trentm trentm commented Nov 23, 2023

Closes: #1782


@pichlermarc I wasn't sure if this should count as a feature or a fix. You had put the "enhancement" label on the issue, so I used "feat" in the commit/PR title.

Update: As pointed out below this change could be a breaking change if one relied on the instrumentationScope.name value.

@trentm trentm self-assigned this Nov 23, 2023
@trentm trentm requested a review from a team November 23, 2023 21:32
@github-actions github-actions bot requested a review from legendecas November 23, 2023 21:33
…ntrib repo requirements on changelog entries
@codecov
Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Merging #1822 (9793602) into main (8f2a195) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1822   +/-   ##
=======================================
  Coverage   91.49%   91.49%           
=======================================
  Files         144      144           
  Lines        7388     7388           
  Branches     1474     1474           
=======================================
  Hits         6760     6760           
  Misses        628      628           
Files Coverage Δ
...ages/opentelemetry-host-metrics/src/BaseMetrics.ts 100.00% <100.00%> (ø)

}

const DEFAULT_NAME = 'opentelemetry-host-metrics';
const DEFAULT_NAME = '@opentelemetry/host-metrics';
Copy link
Member

Choose a reason for hiding this comment

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

This could be potentially a breaking change if someone is already relying on this, we should mention that in the changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC the changelog will automatically get this PR title added to it for the next release. However,

  1. that doesn't include the explicit strings, so won't be as helpful as possible.
  2. Do you think I should add the bang (feat(host-metrics)!: ...) so that this gets added to the "breaking change" section and triggers a new major version (0.34.0)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think 0.34.0 should be used for next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR title to add the !. I hope that is sufficient -- i.e. that the PR title and not the original comment message title is used.

@trentm trentm changed the title feat(host-metrics): use the package name as the default instrumentation scope name, to align with instrumentations feat(host-metrics)!: use the package name as the default instrumentation scope name, to align with instrumentations Nov 27, 2023
Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

LGTM

@trentm trentm enabled auto-merge (squash) December 7, 2023 00:11
@trentm trentm merged commit bcf3501 into open-telemetry:main Dec 7, 2023
@trentm trentm deleted the tm-host-metrics-instrumentation-scope-name branch December 7, 2023 00:12
@dyladan dyladan mentioned this pull request Dec 7, 2023
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.

[host-metrics] adapt default meter name to be the npm package name

3 participants