Skip to content

Conversation

gggritso
Copy link
Member

@gggritso gggritso commented Jun 18, 2025

Very similar to #4796, but for the span.description attribute. Supports Span Field Transition.

V2 spans probably will still have a description, but OTLP spans will not. To make the transition easier, and to create a better experience in the span waterfall, this PR derives a description for HTTP and database spans based on the logic that exists in the JavaScript SDK. It's slightly different from the SDK logic, but not in any important way, from what I can tell.

  1. If the payload includes a description, obeys the description as sent, even if it's an empty string, and sets it as the sentry.description attribute
  2. If the payload does not include a description field, generate a description and put it in the sentry.description attribute
  3. If we cannot determine a good description, fall back to using the name field, since it's similar in purpose

I also removed the little bit of current description processing, since it all lives near the op processing now.

@gggritso gggritso requested a review from loewenheim June 18, 2025 21:14
@gggritso gggritso marked this pull request as ready for review June 18, 2025 21:14
@gggritso gggritso requested a review from a team as a code owner June 18, 2025 21:14
Comment on lines 300 to 304
let is_cache_op = attributes
.get("sentry.op")
.and_then(|attr| attr.value())
.and_then(|attr_val| attr_val.value.value.value())
.and_then(|v| v.as_str())
Copy link
Member Author

Choose a reason for hiding this comment

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

This pattern is starting to show up a lot. Maybe worth adding a method like dig to Attributes to pull this data out?

Copy link
Contributor

Choose a reason for hiding this comment

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

See #4838.

@gggritso
Copy link
Member Author

@AbhiPrasad I borrowed more logic from the JavaScript SDK for this, but also made some changes to comply better to what I was seeing in the recent OTel semantic conventions. Could you take a quick look and see if it looks reasonable? My understanding is that I don't have to match the SDKs 1:1, is that fair?

Comment on lines 300 to 304
let is_cache_op = attributes
.get("sentry.op")
.and_then(|attr| attr.value())
.and_then(|attr_val| attr_val.value.value.value())
.and_then(|v| v.as_str())
Copy link
Contributor

Choose a reason for hiding this comment

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

See #4838.

gggritso and others added 5 commits June 23, 2025 11:31
Co-authored-by: Sebastian Zivota <[email protected]>
Co-authored-by: Sebastian Zivota <[email protected]>
Co-authored-by: Sebastian Zivota <[email protected]>
Co-authored-by: Sebastian Zivota <[email protected]>
gggritso added 3 commits June 23, 2025 11:52
Allow the inferring function to return `None`, and remove checks from
parent. Also add a test for this.
@gggritso
Copy link
Member Author

@loewenheim thanks for the review! I followed pretty much all of your suggestions

  1. Removed redundant checks, and pushed logic down into derive_http_description and derive_db_description
  2. Used your suggestions for simplifying conditionals, splits, etc.
  3. Removed redundant comments, and added a few actually useful ones

There are just a few things left:

  • I changed the kind of some spans in specs because our HTTP inference now only runs if kind is either "client" or "server". I thought the current test cases are useful as-is, so I updated them, but instead I can create new test cases
  • ref: Introduce Attributes struct #4839 looks like it's close, so I'm going to wait for it to land

@loewenheim
Copy link
Contributor

@gggritso In the test case I was referring to, you're changing the kind from client to server. Does that in particular serve a purpose?

@gggritso
Copy link
Member Author

@loewenheim yep, IMO it makes it a more realistic test. The http.route attribute is required on "server" spans, and should not be present on "client" spans, so I updated the kind to match what's in the attributes

Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

There are still some to_string left in v2_to_v1.rs, otherwise LGTM!

@loewenheim
Copy link
Contributor

#4839 has been merged, that should simplify some things.

@gggritso
Copy link
Member Author

@loewenheim great! I fixed all the to_string and updated the code to use Attributes 👍🏻 thanks for that improvement

@gggritso gggritso merged commit ed3f140 into master Jun 24, 2025
44 of 47 checks passed
@gggritso gggritso deleted the georgegritsouk/ope-37-derive-spandescription-for-otlp-spans branch June 24, 2025 16:48
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