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

refactor(examples/opentelemetry-web): use new exported string constants for semconv #4764

Merged

Conversation

Zen-cronic
Copy link
Contributor

Which problem is this PR solving?

Updates #4567

Short description of the changes

Added usage of @opentelemetry/semantic-conventions and @opentelemetry/resources to the examples in examples/opentelemetry-web for maintaining consistency across all examples.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • npm test

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@Zen-cronic Zen-cronic requested a review from a team as a code owner June 4, 2024 16:11
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.86%. Comparing base (ecc88a3) to head (96cca4e).
Report is 35 commits behind head on main.

Current head 96cca4e differs from pull request most recent head 233dc51

Please upload reports for the commit 233dc51 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4764      +/-   ##
==========================================
- Coverage   91.04%   90.86%   -0.18%     
==========================================
  Files          89       91       +2     
  Lines        1954     1960       +6     
  Branches      416      417       +1     
==========================================
+ Hits         1779     1781       +2     
- Misses        175      179       +4     

see 3 files with indirect coverage changes

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Sorry for the churn, the Changelog now has conflicts to resolve because there's been a release. The last one I looked at I was able to merge taking in both incoming and current changes, then move the changelog entry up into the Unreleased section.

@JamieDanielson
Copy link
Member

Reviewing these examples I'm realizing they need some work as they don't run as specified. This change certainly doesn't make things worse, but now I'm wondering if they should be updated wholesale all at once 🤔.

I'm inclined to go ahead with this change to add the service name to these examples since the work is already done (thank you for working on this!), and then separately create an issue to update this example directory.

@trentm @pichlermarc curious of your thoughts here.

@Zen-cronic
Copy link
Contributor Author

Sorry for the churn, the Changelog now has conflicts to resolve because there's been a release. The last one I looked at I was able to merge taking in both incoming and current changes, then move the changelog entry up into the Unreleased section.

no worries!

@trentm
Copy link
Contributor

trentm commented Jun 8, 2024

I'm inclined to go ahead with this change [...] and then separately create an issue to update this example directory.

My inclination is the same.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

LGTM. I suppose this update wasn't necessary, because the preceding state wasn't using the semconv package at all. However, it is still a reasonable update.

@pichlermarc
Copy link
Member

I'm inclined to go ahead with this change to add the service name to these examples since the work is already done (thank you for working on this!), and then separately create an issue to update this example directory.

@trentm @pichlermarc curious of your thoughts here.

Also fine with this 👍

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.

None yet

4 participants