add request to transaction

[jessfraz]

This allows the request information to be sent with the transaction data, which then means http.method etc automatically get populated on the Sentry side. Let me know if you want this to be a different way or if I am missing something. Like I was unsure what to do about Span and I'm not sure this is correct but I know it works for Transaction

[codecov-commenter]

Codecov Report

Merging #439 (fb70909) into master (0009a2d) will decrease coverage by 0.31%.
The diff coverage is 28.30%.

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
- Coverage   80.48%   80.16%   -0.32%     
==========================================
  Files          73       73              
  Lines        8302     8355      +53     
==========================================
+ Hits         6682     6698      +16     
- Misses       1620     1657      +37     

[relaxolotl]

Thanks for opening this PR! It looks like you've already started manually setting performance monitoring/tracing with the SDK, which is great to see 🎉 Before we move forward with the change I'd like to ask two questions first:

1. Would you happen to be using tower already? The SDK has an integration for it that automatically attaches requests to transactions for you, if so.
2. If not, have you taken a look at the SDK's event processor example? It outlines how you would attach a request to a transaction (which is just a type of event) in the SDK.

If neither of those work for you, I'm curious to hear a little bit more about the use case you have for this particular piece of functionality, and how the aforementioned options might not be a good fit for that. Using event processors to attach requests to transactions (and spans) is generally preferred over to exposing a setter on the transaction itself. Going a little bit further, if an integration can take care of that for you, we also prefer making the integration do the job for you instead (see for example #397).

[Swatinem]

1. The tower integration sets the request details on Events, via an event processor.
2. The Rust SDK does not follow the notion of "Transaction = Event + Span", as there is no (multiple) inheritance. We thus do not pass transactions to event processors at all, because they are a distinct type.

The code lgtm, though I wouldn’t attach the request to spans. Rather follow a span to its parent transaction and add it there. We would have to bikeshed this, figuring out how that concept is supposed to work, on which object it is supposed to live (or not), etc.

[dashed]

Regarding the ambiguity of attaching request to a span, we unfortunately do not surface this in the product as we do not expect span.request to actually exist.

Some SDKs such as the JavaScript SDK will add a tag to the span with an http status code:

See: https://github.com/getsentry/sentry-javascript/blob/caba96ef557464283b983466e5e490f3545d8c41/packages/tracing/src/span.ts#L213-L220

A similar approach could be done here by cherry picking useful information from request and attaching it as tag values onto the span.

[relaxolotl]

The Rust SDK does not follow the notion of "Transaction = Event + Span", as there is no (multiple) inheritance. We thus do not pass transactions to event processors at all, because they are a distinct type.

This seems like something the SDK itself should fix. Could we not use some sort of union type to represent both events and transactions, and abstract away the difference between the two at least on an event processor? This would imply that we'd need the concept of a "transaction processor".

It seems like continuing to have transactions and events separated this way via types would have other knock-on side effects similar to the above: parts of the code related an event's lifecycle would potentially need to have a transaction equivalent, as the API is designed and implemented in the other SDKs with the assumption that transactions are loosely equivalent to events.

Regarding storing requests in spans: The spec for a Span suggests that such data should be inserted into its data. I think this is a solid guideline for what we should do here, especially if a user's spans and transactions are modeled in a way that a span captures a uniquely different request than that of a transaction.

[jessfraz]

Ah yeah let me explain a bit... apologies in advance for a bit of a novel.

So on other projects that we have, specifically next.js, we use the sentry next.js SDK and it is amazing. But basically I was looking for the same experience in Rust. So for example, in our next.js projects using the withSentry function on API endpoints we get spans that show up in the UI like the following:

Notice specifically HTTP method is filled in the table.

Then if you click into the span you get this really cool request data:

Notice it even has like the URL and stuff.

So on another project (in rust) I was like I want it to do the same. So I took a look at the tower integration and wrote a kinda bespoke handler for our HTTP server we are using which is https://github.com/oxidecomputer/dropshot, Anyways the code is here: https://github.com/oxidecomputer/cio/blob/master/webhooky/src/server.rs#L2634 and it is very similar to the code from tower. But before this patch in this PR the behavior was the following:

In the sentry UI it would show up like:

notice HTTP method is not filled in, neither is any of the cool request data if you click into the event for the span

So I went into the withSentry code for the nextjs SDK and compared it to what I was doing to figure out wtf the problem was since I was doing an event processor and everything seemed kosher. And these lines right here: https://github.com/getsentry/sentry-javascript/blob/master/packages/nextjs/src/utils/withSentry.ts#L68 seemed to be the main difference.

So then I was like, I need to send the request with the transaction.

Which leads me to this patch. However with this patch it is still not perfectly like the behavior of the nextjs SDK, but maybe there is something else I am missing. Basically this patch makes the HTTP method show up like:

So we are good there. And if I click into the event for the span I get header details and other details of the request which is great:

but if you look at the difference from the nextjs screen shot of the event details like the URL and stuff at the top is not populated but maybe that information comes from somewhere else. I'm not an expert by any means on the sentry SDK but some guidance would be great, thanks!

[relaxolotl]

Thanks for the thorough write up; That's some solid sleuthing work. I'll quickly address the PR's change first: The diff looks good, although there's one property that should be removed from the protocol. The existing tests (see test_tracing.rs's set_transaction) ideally should be updated as well. Extending the performance demo (examples/performance-demo.rs) to provide an example of how/when this should be used would be appreciated as well, but I wouldn't consider it a hard requirement for this change. Feel free to poke us if you need any help in adding these in.

To address the one discrepancy you've noticed:

if you look at the difference from the nextjs screen shot of the event details like the URL and stuff at the top is not populated but maybe that information comes from somewhere else.

Based on the code for the UI, both a non-empty method and a valid, non-empty url are what's needed to get the missing title to show up. What makes up a valid URL can be found here, but the short answer is that it needs to start with http[s]://.

I did a bit of testing of these changes locally and everything looks great, and the title's nicely populated when the two aforementioned requirements are met. I'd be curious to hear whether this helps resolve the missing title problem you're running into, however it shouldn't block the changes in this PR.

I believe there's a second conversation regarding whether this is the correct API to expose in the SDK for those wishing to include request data with their transactions and spans. I don't think that conversation should block what you're doing here, so I'm yanking that out into a separate issue for the team to discuss. The API in this PR may be changed as a result of that conversation, but we'll try to give you (and others) some form of a heads up when that happens.

[relaxolotl]

Since you're inserting the request's contents into data, this should be removed. (This'll likely be pruned or rejected by relay, if it slips in.)

[jessfraz]

ah thanks!

[jessfraz]

Oh thanks this is helpful also I didn't realize the UI is open source as well, that's awesome!

[jessfraz]

Let me know if you want me to squash all these commits into one, was leaving them just so its easier to review

[relaxolotl]

@jessfraz I've got you covered on the squashing 👍 PR commits are usually squashed in sentry repositories anyways, so I've gone ahead and did that for you - apologies if that stepped on your toes a bit too much though!

[Swatinem]

looks like I’m too late for the party ;-)

Would be nice to get this support into the actix and tower (and by extension, axum) integrations as well.

[Swatinem]

Looks like a typo, that should be set_request ;-)