Skip to content

Conversation

lcian
Copy link
Member

@lcian lcian commented Aug 28, 2025

Description

This changes how the tracing integration works with regards to span names and operations.
(In the analysis below, I assume the spans are all created with the tracing::instrument macro)

Previous behavior

  • Sentry span name = <module>::<function>
    • unless a message field is set, in which case it will determine the Sentry span name
    • this message field is arbitrary, not documented, and doesn't align with our conventions
  • Sentry op = function name
Screenshot 2025-08-28 at 16 16 01

Behavior after this PR

  • Sentry span name = tracing span name (= function name, when using the tracing::instrument macro)
  • Sentry op = default, as we have no way to infer an op that makes sense - it will be hidden in the UI
  • Users can set 3 additional fields that have special meaning for our tracing Subscriber and override values on the corresponding Sentry span:
    • sentry.name
    • sentry.op
    • sentry.trace: while not related to this issue, I noticed that there is confusion on how one would achieve distributed tracing. This makes it possible without having to use the Sentry tracing API and understand the interplay between Sentry and the tracing crate.
Screenshot 2025-08-28 at 16 15 51

Advantages of the change

  • The new conversion is more aligned with Sentry conventions, documented, and gives additional capabilities to users
  • It's also similar to the approach taken by tracing-opentelemetry, which offers a otel.name special property

Drawbacks of the change

  • We break existing customers that might be using span metrics/dashboards/queries based on the old span name - they will have to change them
  • If you have 2 functions with the same name in different modules, they will now have the same span name. We could mitigate this by writing the info from span.metadata.target into span attributes, like we do for logs to surface function and line

Closes

Close #880
Close #879

@lcian lcian changed the base branch from master to lcian/test/fix-doctests August 28, 2025 13:20
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 72.15190% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.23%. Comparing base (cd933d3) to head (ef97dc7).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
- Coverage   73.28%   73.23%   -0.05%     
==========================================
  Files          64       64              
  Lines        7407     7473      +66     
==========================================
+ Hits         5428     5473      +45     
- Misses       1979     2000      +21     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Base automatically changed from lcian/test/fix-doctests to master August 28, 2025 13:22
lcian added 2 commits August 28, 2025 15:25
Corrected punctuation in the CHANGELOG regarding Sentry span overrides.
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

nice, this looks a lot cleaner indeed.

I think its fair to use just the function name, from the users perspective, I think that makes sense. Although I also might want to see the fully qualified name sometimes, in particular for method names.
its a bit unfortunate that tracing does not differentiate between an impl method and a free function here.
Ideally, it should qualify methods with the impl (such as Vec::extend_from_slice).
I think not qualifying anything could lead to confusion for example if you have str::from_utf8 vs String::from_utf8, which are two different methods.

But okay, I guess that is a minor annoyance.

@lcian
Copy link
Member Author

lcian commented Aug 29, 2025

Thanks @Swatinem, that's a fair point.
We could still report the fully qualified name as the op, so the information in the UI would be the same as before but swapped. Maybe that's the way to go, as default doesn't bring any value to the user anyway, might as well "abuse" the field to report the full name.

@lcian lcian marked this pull request as ready for review August 29, 2025 12:32
@Swatinem
Copy link
Member

One thing to try would be whether it actually is the "fully qualified" name, because just concatenating the module to the name, does it also include the type name for impl methods?

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.

tracing::span "message" field should NOT be empty during span creation Sentry span.description requires setting "message" field
2 participants