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

feat: updated spec to v1.5.0 and renamed resource class #2345

Merged
merged 15 commits into from
Jul 25, 2021

Conversation

weyert
Copy link
Contributor

@weyert weyert commented Jul 14, 2021

Which problem is this PR solving?

Resolves ticket #2299

Short description of the changes

  • Updated the specification to the latest version 1.5.0 so the latest semantic convention attributes will be available (e.g. mobile ones)

  • Renamed the ResourceAttributes to SemanticResourceAttributes

Updated the specification to the latest version 1.5.0

Renamed the `ResourceAttributes` to `SemanticResourceAttributes`

fixes open-telemetry#2299
@dyladan
Copy link
Member

dyladan commented Jul 14, 2021

19 seconds old PR already has conflicts 😆

@weyert
Copy link
Contributor Author

weyert commented Jul 14, 2021

Hmm, I don't understand why it can not find Error: Cannot find module '@opentelemetry/semantic-conventions' in the opentracing-shim package

@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

Merging #2345 (1ac0fa6) into main (884d20a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2345      +/-   ##
==========================================
- Coverage   92.77%   92.76%   -0.02%     
==========================================
  Files         145      145              
  Lines        5221     5221              
  Branches     1070     1070              
==========================================
- Hits         4844     4843       -1     
- Misses        377      378       +1     
Impacted Files Coverage Δ
...s/opentelemetry-core/src/platform/node/sdk-info.ts 100.00% <ø> (ø)
...detector-aws/src/detectors/AwsBeanstalkDetector.ts 95.65% <ø> (ø)
...ource-detector-aws/src/detectors/AwsEc2Detector.ts 97.91% <ø> (ø)
...resource-detector-gcp/src/detectors/GcpDetector.ts 95.83% <ø> (ø)
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts 90.90% <100.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 100.00% <100.00%> (ø)
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 100.00% <100.00%> (ø)
...ource-detector-aws/src/detectors/AwsEksDetector.ts 91.25% <100.00%> (ø)
...ce-detector-aws/src/detectors/AwsLambdaDetector.ts 100.00% <100.00%> (ø)
packages/opentelemetry-resources/src/Resource.ts 100.00% <100.00%> (ø)
... and 3 more

@weyert
Copy link
Contributor Author

weyert commented Jul 14, 2021

I am not sure why the browser test is failing

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, just missing one thing to update main readme.md with information about breaking changes

@obecny
Copy link
Member

obecny commented Jul 15, 2021

creating a corresponding issue in contrib repo open-telemetry/opentelemetry-js-contrib#574

@weyert
Copy link
Contributor Author

weyert commented Jul 15, 2021

lgtm, just missing one thing to update main readme.md with information about breaking changes

Good point, I will update it first thing tomorrow :)

@weyert
Copy link
Contributor Author

weyert commented Jul 16, 2021

Added the READMe.md change @obecny

@obecny
Copy link
Member

obecny commented Jul 16, 2021

Added the READMe.md change @obecny

can you revert the formatting it had ?

@CptSchnitz
Copy link
Contributor

Wouldn't making the changes in the examples break all of them until the release of the semantic conventions package with the changes?

@weyert
Copy link
Contributor Author

weyert commented Jul 16, 2021

Updated :)

Think I finally managed to disable Prettier for this project in Visual Studio Code

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for updating the README header as well

@dyladan
Copy link
Member

dyladan commented Jul 21, 2021

One more approval and please fix the conflict and this will be good to go

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@vmarchaud
Copy link
Member

@weyert i merged main into the PR but didn't change a quote sorry, could you fix 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.

7 participants