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

Add peer.service attribute to psr18 spans #303

Closed
wants to merge 1 commit into from

Conversation

caetanev
Copy link
Contributor

@caetanev caetanev commented Oct 5, 2024

The peer.service attribute helps to relate traces between services. The attribute is defined in the semantic conventions: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/attributes-registry/peer.md?plain=1#L15

Add the peer.service attribute to the Psr18 spans that request known services.

@caetanev caetanev requested a review from a team as a code owner October 5, 2024 09:26
Copy link

linux-foundation-easycla bot commented Oct 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: caetanev / name: Evandro Caetano (838ab59)

Copy link

codecov bot commented Oct 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.62%. Comparing base (67ce817) to head (838ab59).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #303      +/-   ##
============================================
+ Coverage     80.33%   81.62%   +1.29%     
+ Complexity     1026      995      -31     
============================================
  Files            98       94       -4     
  Lines          4114     4050      -64     
============================================
+ Hits           3305     3306       +1     
+ Misses          809      744      -65     
Flag Coverage Δ
Aws 85.51% <ø> (ø)
Context/Swoole ?
Instrumentation/CakePHP 20.00% <ø> (ø)
Instrumentation/CodeIgniter 73.94% <ø> (ø)
Instrumentation/ExtAmqp 89.58% <ø> (ø)
Instrumentation/Guzzle 69.73% <ø> (ø)
Instrumentation/HttpAsyncClient 81.33% <ø> (ø)
Instrumentation/IO 70.90% <ø> (ø)
Instrumentation/MongoDB 77.33% <ø> (ø)
Instrumentation/OpenAIPHP 86.82% <ø> (ø)
Instrumentation/PDO 89.56% <ø> (ø)
Instrumentation/Psr14 78.12% <ø> (ø)
Instrumentation/Psr15 93.50% <ø> (ø)
Instrumentation/Psr16 97.50% <ø> (ø)
Instrumentation/Psr18 82.35% <100.00%> (+0.26%) ⬆️
Instrumentation/Psr3 60.25% <ø> (ø)
Instrumentation/Psr6 97.61% <ø> (ø)
Instrumentation/Slim 86.95% <ø> (ø)
Instrumentation/Symfony 89.07% <ø> (ø)
Instrumentation/Yii 77.77% <ø> (ø)
Logs/Monolog 100.00% <ø> (ø)
Propagation/ServerTiming 100.00% <ø> (ø)
Propagation/TraceResponse 100.00% <ø> (ø)
ResourceDetectors/Container 93.02% <ø> (ø)
Sampler/RuleBased 32.14% <ø> (ø)
Shims/OpenTracing 92.99% <ø> (ø)
Symfony 88.20% <ø> (ø)

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

Files with missing lines Coverage Δ
...Instrumentation/Psr18/src/Psr18Instrumentation.php 80.95% <100.00%> (+0.30%) ⬆️

... and 4 files with indirect coverage changes


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 67ce817...838ab59. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented Oct 6, 2024

SHOULD be equal to the actual service.name resource attribute of the remote service if any.

This change would apply to all http requests, which may or may not have a service.name. What are other SIGs doing here?

@ChrisLightfootWild
Copy link
Contributor

SHOULD be equal to the actual service.name resource attribute of the remote service if any.

This change would apply to all http requests, which may or may not have a service.name. What are other SIGs doing here?

Looks like Java went with mapping services via OTEL_INSTRUMENTATION_COMMON_PEER_SERVICE_MAPPING:
https://opentelemetry.io/docs/zero-code/java/agent/instrumentation/

This looks like it would suffer the same problem but was recently accepted: #302

Copy link
Collaborator

@bobstrecansky bobstrecansky left a comment

Choose a reason for hiding this comment

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

We should follow the Java SIG pattern here.

@bobstrecansky bobstrecansky self-requested a review October 7, 2024 19:11
@bobstrecansky
Copy link
Collaborator

Removed my review - inadvertently approved.

@brettmc
Copy link
Collaborator

brettmc commented Oct 7, 2024

I think Java's approach is a great one to follow - and I also remember now that it's supported in yaml-based config: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml#L384

This looks like it would suffer the same problem but was recently accepted: #302

Ugh, sorry I didn't think about that during review. Once we get a pattern we're happy with in this PR, can apply the same to Symfony...

@caetanev
Copy link
Contributor Author

caetanev commented Oct 8, 2024

I will close this PR and work towards replicating the Java SIG solution.

@caetanev caetanev closed this Oct 8, 2024
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