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

Update TraceAttributes to semconv to 1.22.0 #204

Merged

Conversation

julianocosta89
Copy link
Member

After PR open-telemetry/opentelemetry-php#1126 got merged, HTTP_FLAVOR was dropped and all the instrumentations that used it started failing.

This PR updates TraceAttributes::HTTP_FLAVOR to TraceAttributes::NETWORK_PROTOCOL_VERSION.

@julianocosta89 julianocosta89 requested a review from a team October 19, 2023 09:33
@welcome
Copy link

welcome bot commented Oct 19, 2023

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

@brettmc
Copy link
Collaborator

brettmc commented Oct 19, 2023

Thanks, @julianocosta89 - I've just noticed that I only check for removed ResourceAttributes but not TraceAttributes as part of updating SemConv...
There are a few failures still, it looks like some packages are not picking up the latest SemConv version - they might need a more explicit version constraint. I think ^1.22 should do it.

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #204 (8ab28c7) into main (2439ee9) will not change coverage.
The diff coverage is 13.95%.

❗ Current head 8ab28c7 differs from pull request most recent head d24f361. Consider uploading reports for the commit d24f361 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #204   +/-   ##
=========================================
  Coverage     33.13%   33.13%           
  Complexity      852      852           
=========================================
  Files            76       76           
  Lines          3247     3247           
=========================================
  Hits           1076     1076           
  Misses         2171     2171           
Flag Coverage Δ
7.4 46.34% <80.00%> (ø)
8.0 33.06% <13.95%> (ø)
8.1 33.10% <13.95%> (ø)
8.2 33.08% <13.95%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...fony/src/OtelBundle/HttpKernel/RequestListener.php 77.34% <100.00%> (ø)
...n/MongoDB/src/MongoDBInstrumentationSubscriber.php 0.00% <0.00%> (ø)
...entation/Symfony/src/HttpClientInstrumentation.php 0.00% <0.00%> (ø)
...ion/CodeIgniter/src/CodeIgniterInstrumentation.php 0.00% <0.00%> (ø)
...Instrumentation/Psr15/src/Psr15Instrumentation.php 0.00% <0.00%> (ø)
...c/Instrumentation/Slim/src/SlimInstrumentation.php 0.00% <0.00%> (ø)
...rumentation/Symfony/src/SymfonyInstrumentation.php 0.00% <0.00%> (ø)
...tion/Laravel/src/Watchers/ClientRequestWatcher.php 0.00% <0.00%> (ø)
...AsyncClient/src/HttpAsyncClientInstrumentation.php 0.00% <0.00%> (ø)
...Instrumentation/Psr18/src/Psr18Instrumentation.php 0.00% <0.00%> (ø)
... and 1 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2439ee9...d24f361. Read the comment docs.

@julianocosta89
Copy link
Member Author

Thanks @brettmc!

It seems that there are other attributes that need to be updated, I'll take care of that

@julianocosta89
Copy link
Member Author

julianocosta89 commented Oct 19, 2023

Just one attribute that I couldn't find: HTTP_CLIENT_IP

I've replaced with NETWORK_PEER_ADDRESS, but not sure if that is correct.

@julianocosta89
Copy link
Member Author

@brettmc and @pdelewski I've updated a bunch of other TraceAttributes after your review, would you mind double checking it?
Also, I'm not sure about this: #204 (comment)

Appreciate the help!

@julianocosta89 julianocosta89 changed the title Replace HTTP_FLAVOR with NETWORK_PROTOCOL_VERSION Update TraceAttributes to semconv to 1.22.0 Oct 19, 2023
Copy link
Contributor

@cedricziel cedricziel left a comment

Choose a reason for hiding this comment

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

lgtm!

@brettmc brettmc merged commit a66b1f1 into open-telemetry:main Oct 19, 2023
58 checks passed
@brettmc
Copy link
Collaborator

brettmc commented Oct 19, 2023

I've tagged new versions of the updated packages. Thanks, @julianocosta89

@julianocosta89 julianocosta89 deleted the Update-SemConv-TraceAttribute branch October 19, 2023 20:39
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.

4 participants