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

Use semantic convention constants #964

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

fractalwrench
Copy link
Contributor

Goal

Replaces existing OTel semantic convention keys with the constants defined in the semantic convention dependency.

Note: as part of this I noticed that os.model.identifier and os.model.name were mistakenly set as a resource value - they should have been prefixed with device. I've chosen to fix this as I believe it will only affect OTel export & doesn't need changes on our backend.

Testing

Relied on existing test coverage.

@fractalwrench fractalwrench requested a review from a team as a code owner June 14, 2024 13:19
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.91%. Comparing base (d194ec6) to head (9ff6570).

Current head 9ff6570 differs from pull request most recent head 45771b0

Please upload reports for the commit 45771b0 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                      @@
##           use-semantic-conventions     #964      +/-   ##
============================================================
- Coverage                     80.91%   80.91%   -0.01%     
============================================================
  Files                           445      443       -2     
  Lines                         11827    11790      -37     
  Branches                       1806     1802       -4     
============================================================
- Hits                           9570     9540      -30     
+ Misses                         1462     1459       -3     
+ Partials                        795      791       -4     
Files Coverage Δ
...droid/embracesdk/arch/destination/LogWriterImpl.kt 95.23% <100.00%> (ø)
...droid/embracesdk/internal/spans/EmbraceSpanImpl.kt 86.71% <100.00%> (ø)
...acesdk/opentelemetry/OpenTelemetryConfiguration.kt 96.77% <100.00%> (ø)
...roid/embracesdk/internal/logs/EmbraceLogService.kt 94.69% <80.00%> (ø)
...id/embracesdk/capture/crash/CrashDataSourceImpl.kt 90.32% <71.42%> (-1.21%) ⬇️

... and 13 files with indirect coverage changes

Copy link
Collaborator

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

Oops! Yeah, those keys are not used by us and in fact the resource object isn't even sent to our backend via the envelope. We probably should though - the resource object in the payload envelope should be a superset of this.

The reason this wasn't done was because the the attribute naming. Our backend doesn't like the . in the key names, so we never merged the two. There's an outstanding item to resolve this that we were going to do after we shipped our first version.... which is now.

More details about this here: https://www.notion.so/embraceio/How-do-we-name-payload-envelope-fields-that-are-also-OTel-attributes-and-resources-given-the-conflic-4e8bc1ea3a4c457c9243774ada475c90?pvs=4

Copy link
Collaborator

@bidetofevil bidetofevil left a comment

Choose a reason for hiding this comment

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

LGTM

Use further semantic convention constants
@fractalwrench fractalwrench merged commit 143d486 into use-semantic-conventions Jun 17, 2024
1 check passed
@fractalwrench fractalwrench deleted the adopt-sem-conv-constants branch June 17, 2024 09:38
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

2 participants