Skip to content

Conversation

Nevay
Copy link
Contributor

@Nevay Nevay commented May 17, 2025

Alternative to #1583

Span suppression is implemented in the SDK -> instrumentation does not need to handle suppression.
(Everything besides SemanticConvention and SemanticConventionResolver could be moved to the SDK.)

Provides three span suppression strategies:
Noop: does not suppress anything
SpanKind: suppresses nested spans with the same span kind (except for internal)
SemConv: attempts to resolve the semantic convention based on span attributes, otherwise falls back to span kind suppression

Copy link
Contributor

@brettmc brettmc left a comment

Choose a reason for hiding this comment

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

This looks pretty solid. There's a lot of configuration, it seems like this will need to use declarative configuration, but that can be a follow-up change.

@brettmc
Copy link
Contributor

brettmc commented May 22, 2025

I'm happy to accept this as-is and complete it incrementally, if you don't have much time. If you're happy with that, can you add @experimental tags everywhere, and fix the (hopefully minor) CI issues. I've documented some steps to get this towards readiness in #1579

Copy link

codecov bot commented May 25, 2025

Codecov Report

❌ Patch coverage is 77.27273% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.36%. Comparing base (00417cf) to head (f05250b).

Files with missing lines Patch % Lines
...cConventionSuppressionStrategy/WildcardPattern.php 0.00% 12 Missing ⚠️
...ppressionStrategy/SemanticConventionSuppressor.php 81.48% 5 Missing ⚠️
src/SDK/Trace/TracerProviderBuilder.php 0.00% 4 Missing ⚠️
...c/API/Trace/SpanSuppression/SemanticConvention.php 0.00% 2 Missing ⚠️
src/SDK/Trace/Span.php 50.00% 2 Missing ⚠️
...onStrategy/CompiledSemanticConventionAttribute.php 0.00% 2 Missing ⚠️
...SpanKindSuppressionStrategy/SpanKindSuppressor.php 77.77% 2 Missing ⚠️
src/SDK/Trace/SpanBuilder.php 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1599      +/-   ##
============================================
+ Coverage     68.24%   68.36%   +0.12%     
- Complexity     2919     2978      +59     
============================================
  Files           435      447      +12     
  Lines          8876     9008     +132     
============================================
+ Hits           6057     6158     +101     
- Misses         2819     2850      +31     
Flag Coverage Δ
8.1 68.01% <77.27%> (+0.18%) ⬆️
8.2 68.15% <77.27%> (+0.02%) ⬆️
8.3 68.25% <77.27%> (+0.20%) ⬆️
8.4 68.22% <77.27%> (+0.14%) ⬆️
8.5 68.21% <77.27%> (+0.17%) ⬆️

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

Files with missing lines Coverage Δ
src/API/Trace/Span.php 94.44% <ø> (ø)
...ession/NoopSuppressionStrategy/NoopSuppression.php 100.00% <100.00%> (ø)
...oopSuppressionStrategy/NoopSuppressionStrategy.php 100.00% <100.00%> (ø)
...ression/NoopSuppressionStrategy/NoopSuppressor.php 100.00% <100.00%> (ø)
...pressionStrategy/SemanticConventionSuppression.php 100.00% <100.00%> (ø)
...Strategy/SemanticConventionSuppressionStrategy.php 100.00% <100.00%> (ø)
...panKindSuppressionStrategy/SpanKindSuppression.php 100.00% <100.00%> (ø)
...uppressionStrategy/SpanKindSuppressionStrategy.php 100.00% <100.00%> (ø)
src/SDK/Trace/Tracer.php 89.47% <100.00%> (+0.58%) ⬆️
src/SDK/Trace/TracerProvider.php 100.00% <100.00%> (ø)
... and 8 more

... 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 00417cf...f05250b. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines 26 to 27
new SemanticConvention('span.http.client', SpanKind::KIND_CLIENT, ['http.request.method', 'server.address', 'server.port', 'url.full'], ['network.peer.address', 'network.peer.port', 'error.type', 'http.request.body.size', 'http.request.header', 'http.request.method_original', 'http.request.resend_count', 'http.request.size', 'http.response.body.size', 'http.response.header', 'http.response.size', 'http.response.status_code', 'network.protocol.name', 'network.protocol.version', 'network.transport', 'user_agent.original', 'user_agent.synthetic.type', 'url.scheme', 'url.template']),
new SemanticConvention('span.http.server', SpanKind::KIND_SERVER, ['http.request.method', 'url.path', 'url.scheme'], ['network.peer.address', 'network.peer.port', 'error.type', 'http.request.body.size', 'http.request.method_original', 'http.request.size', 'http.response.body.size', 'http.response.header', 'http.response.size', 'http.response.status_code', 'network.protocol.name', 'network.protocol.version', 'network.transport', 'user_agent.synthetic.type', 'client.address', 'client.port', 'http.request.header', 'http.route', 'network.local.address', 'network.local.port', 'user_agent.original', 'server.address', 'server.port', 'url.query']),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
new SemanticConvention('span.http.client', SpanKind::KIND_CLIENT, ['http.request.method', 'server.address', 'server.port', 'url.full'], ['network.peer.address', 'network.peer.port', 'error.type', 'http.request.body.size', 'http.request.header', 'http.request.method_original', 'http.request.resend_count', 'http.request.size', 'http.response.body.size', 'http.response.header', 'http.response.size', 'http.response.status_code', 'network.protocol.name', 'network.protocol.version', 'network.transport', 'user_agent.original', 'user_agent.synthetic.type', 'url.scheme', 'url.template']),
new SemanticConvention('span.http.server', SpanKind::KIND_SERVER, ['http.request.method', 'url.path', 'url.scheme'], ['network.peer.address', 'network.peer.port', 'error.type', 'http.request.body.size', 'http.request.method_original', 'http.request.size', 'http.response.body.size', 'http.response.header', 'http.response.size', 'http.response.status_code', 'network.protocol.name', 'network.protocol.version', 'network.transport', 'user_agent.synthetic.type', 'client.address', 'client.port', 'http.request.header', 'http.route', 'network.local.address', 'network.local.port', 'user_agent.original', 'server.address', 'server.port', 'url.query']),
new SemanticConvention('span.http.client', SpanKind::KIND_CLIENT, ['http.request.method', 'server.address', 'server.port', 'url.full'], ['network.peer.address', 'network.peer.port', 'error.type', 'http.request.body.size', 'http.request.header.*', 'http.request.method_original', 'http.request.resend_count', 'http.request.size', 'http.response.body.size', 'http.response.header.*', 'http.response.size', 'http.response.status_code', 'network.protocol.name', 'network.protocol.version', 'network.transport', 'user_agent.original', 'user_agent.synthetic.type', 'url.scheme', 'url.template']),
new SemanticConvention('span.http.server', SpanKind::KIND_SERVER, ['http.request.method', 'url.path', 'url.scheme'], ['network.peer.address', 'network.peer.port', 'error.type', 'http.request.body.size', 'http.request.method_original', 'http.request.size', 'http.response.body.size', 'http.response.header.*', 'http.response.size', 'http.response.status_code', 'network.protocol.name', 'network.protocol.version', 'network.transport', 'user_agent.synthetic.type', 'client.address', 'client.port', 'http.request.header.*', 'http.route', 'network.local.address', 'network.local.port', 'user_agent.original', 'server.address', 'server.port', 'url.query']),

Implementation does not handle template types yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must also add general attributes https://opentelemetry.io/docs/specs/semconv/general/attributes/

The attributes described in this section are not specific to a particular operation but rather generic. They may be used in any Span they apply to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like I forgot to include inherited attributes when generating this resolver (for example db.system.name is missing in all span.db.*.client semconvs), should be fixed as a follow-up.

*/
interface SpanSuppressionStrategy
{
public function getSuppressor(string $name, ?string $version, ?string $schemaUrl): SpanSuppressor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could accept InstrumentationScopeInterface instead.

/**
* @param int<0, 4> $spanKind
*/
public function resolveSuppression(int $spanKind, array $attributes): SpanSuppression;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could accept AttributesInterface instead.

Note: SemanticConventionSuppressor should still operate on the underlying ::toArray() to avoid unnecessary method calls.

@Nevay Nevay force-pushed the feature/span-suppression branch from b0205b3 to 9b3653c Compare June 27, 2025 18:31
@jamespavett
Copy link

jamespavett commented Sep 26, 2025

Is there any movement in this at all?

Currently have a lot of pain when using some of the symfony / psr auto instrumentation bundles for HTTP requests, hope this will alleviate the problem.

See a lot of nested spans that are identical in name that are sometimes 5/6 levels deep.

image

@Nevay Nevay force-pushed the feature/span-suppression branch from 9b3653c to 0afbf7e Compare September 27, 2025 11:52
@Nevay Nevay marked this pull request as ready for review September 27, 2025 12:14
@Nevay Nevay requested a review from a team as a code owner September 27, 2025 12:14
@jamespavett
Copy link

Does this PR expose any way to configure which suppression strategy is used by the SDK when used via autoinstrumentation bundles?

Was expecting to see some environment variable configuration, but haven't seen any.

@brettmc
Copy link
Contributor

brettmc commented Sep 29, 2025

Does this PR expose any way to configure which suppression strategy is used by the SDK when used via autoinstrumentation bundles?
Was expecting to see some environment variable configuration, but haven't seen any.

No. I think that will be a follow-up, which is called out in #1579. I think it's too complicated to work with env vars, and so would require YAML-based declarative config (happy to be proven wrong, though). So step 1, let's get it in, then do some field testing that it works, then make it more useful/accessible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants