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

chore(resources): update semconv usage to modern ATTR_ export names #5187

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Nov 20, 2024

Refs: #4896

status

This is in draft while we discuss how to handle using unstable semconv attributes: (1) use the incubating entry-point or (2) make local copies of the unstable constants. See #5182 (comment)

  • The first commit on this feature branch shows option (1): using the "incubating" entry-point.
  • The third commit shows changing to option (2): making a copy of unstable values (to "src/semconv.ts") and using those.

@trentm trentm self-assigned this Nov 20, 2024
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.59%. Comparing base (e97cc2e) to head (f89b10a).

Files with missing lines Patch % Lines
...try-resources/src/detectors/BrowserDetectorSync.ts 0.00% 1 Missing ⚠️
...lemetry-resources/src/detectors/EnvDetectorSync.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5187      +/-   ##
==========================================
+ Coverage   94.58%   94.59%   +0.01%     
==========================================
  Files         314      315       +1     
  Lines        7993     8008      +15     
  Branches     1611     1611              
==========================================
+ Hits         7560     7575      +15     
  Misses        433      433              
Files with missing lines Coverage Δ
packages/opentelemetry-resources/src/Resource.ts 100.00% <ø> (ø)
...es/src/detectors/platform/node/HostDetectorSync.ts 100.00% <100.00%> (ø)
...rces/src/detectors/platform/node/OSDetectorSync.ts 100.00% <100.00%> (ø)
...src/detectors/platform/node/ProcessDetectorSync.ts 92.85% <100.00%> (ø)
...ors/platform/node/ServiceInstanceIdDetectorSync.ts 57.14% <100.00%> (ø)
packages/opentelemetry-resources/src/semconv.ts 100.00% <100.00%> (ø)
...try-resources/src/detectors/BrowserDetectorSync.ts 52.94% <0.00%> (ø)
...lemetry-resources/src/detectors/EnvDetectorSync.ts 94.11% <50.00%> (ø)
---- 🚨 Try these New Features:

ATTR_CLOUD_REGION,
ATTR_CONTAINER_ID,
ATTR_CONTAINER_IMAGE_NAME,
ATTR_CONTAINER_IMAGE_TAGS,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewer note: A slightly subtle change is that this changed from container.image.tag (singular) to container.image.tags (plural). That was changed in semconv 1.22 (changelog entry), but isn't mentioned in the schema.yaml files.

Note as well that testing for container.image.* is a little weird, because those resource attributes are only used in tests (in this package, in the "sdk-node" package, and in the "test-utils" package in the contrib repo). I think the assertContainerResource() function was written 4+ years ago and then copy-pasta'd to the other two packages' tests.

Note that the 'incubating' entry-point is still used in tests, which
might be a nice way to watch for and catch breaking changes without
breaking runtime code.
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.

1 participant