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 request hostname to span name #305

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NeilWhitworth
Copy link

@NeilWhitworth NeilWhitworth requested a review from a team as a code owner October 11, 2024 22:38
Copy link

welcome bot commented Oct 11, 2024

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.

Copy link

CLA Not Signed

@brettmc
Copy link
Collaborator

brettmc commented Oct 14, 2024

This has come up before in various SIGs across the otel ecosystem, eg open-telemetry/opentelemetry-java-instrumentation#6526 and open-telemetry/opentelemetry-java-instrumentation#8339

The responses I've seen seem to boil down to "no, that's not what the spec says, here are some workarounds". The workarounds can include allowing developers to provide some custom logic to set the span name, or using a custom Span Processor or a collector component.
I'd also suggest another option for our SIG - we have the foundations of file-based configuration, which allows for a much richer configuration of the SDK and auto-instrumentation. So perhaps we could do something with that? I'd be much more comfortable if any deviation from the spec was opt-in.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.64%. Comparing base (67ce817) to head (6a7a2dc).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #305      +/-   ##
============================================
- Coverage     80.33%   79.64%   -0.70%     
+ Complexity     1026      983      -43     
============================================
  Files            98       93       -5     
  Lines          4114     3812     -302     
============================================
- Hits           3305     3036     -269     
+ Misses          809      776      -33     
Flag Coverage Δ
Aws 85.51% <ø> (ø)
Context/Swoole 0.00% <ø> (ø)
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.08% <ø> (ø)
Instrumentation/Psr3 60.25% <ø> (ø)
Instrumentation/Psr6 97.61% <ø> (ø)
Instrumentation/Slim 86.95% <ø> (ø)
Instrumentation/Symfony ?
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.

see 5 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...6a7a2dc. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented Oct 24, 2024

Here's a half-baked idea. I'm currently working on enabling SDK configuration through SPI and wondering if we can use SPI to solve this problem...

How about if we come up with a new API/SDK interface, say something like HttpSpanNameExtractor, with a method like extract(RequestInterface $request): string.
We can build a spec-compliant implementation ({method}) which would be the default, and one that does {method} {host} as in this PR. Using SPI also allows folks to write their own super-customized version as suits their needs, and non-default behaviour is opt-in.

@NeilWhitworth
Copy link
Author

Here's a half-baked idea. I'm currently working on enabling SDK configuration through SPI and wondering if we can use SPI to solve this problem...

How about if we come up with a new API/SDK interface, say something like HttpSpanNameExtractor, with a method like extract(RequestInterface $request): string. We can build a spec-compliant implementation ({method}) which would be the default, and one that does {method} {host} as in this PR. Using SPI also allows folks to write their own super-customized version as suits their needs, and non-default behaviour is opt-in.

It would be useful to have an API interface to control this. Internally we have several Libraries (build from OpenAPI specs) that could build a better name, including the low-cardinality route information, but I could not see any way to do this within the current implementation.

Falling back to using {method} {host} seamed a better option that worked most of the time, and provided more information whilst staying within the spirit of the specs.

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.

2 participants