getsentry / sentry-dart

Sentry SDK for Dart and Flutter
https://sentry.io/for/flutter/
MIT License
746 stars 227 forks source link

Capture HTTP Response Body for `SentryHttpClient` #2220

Open sumanthratna opened 1 month ago

sumanthratna commented 1 month ago

Problem Statement

I would like to automatically capture the following information for all outgoing requests made by SentryHttpClient:

This should be opt-in, similar to networkDetailAllowUrls in the JavaScript SDK: https://docs.sentry.io/platforms/javascript/session-replay/configuration/#network-details

Solution Brainstorm

https://github.com/getsentry/rfcs/blob/main/text/0022-response-context.md

Are you willing to submit a PR?

Yes

buenaflor commented 1 month ago

hey, what did you have in mind where you want this information to show up in the sentry product?

sumanthratna commented 1 month ago

@buenaflor Here is what I'm thinking (based on the accepted RFC linked above):

Each request made through SentryHttpClient should add a transaction within the current trace. Each transaction has a "Request" context and a "Response" context, which hold the requested data (listed above in the bullet points). [For request and response bodies, attachments on the transaction may be more suitable due to size limitations with context.]

Our use-case is that when an HTTP request fails, we don't just want to know which request failed, but we want to see the entire trace of requests and responses that led up to the failing request.

Does this make sense? Thanks!

sumanthratna commented 1 month ago

Another possible way to capture + present this information could be Breadcrumbs (which I believe already support outgoing HTTP requests, but doesn't include request body or response data)

kahest commented 1 month ago

@sumanthratna if I understand correctly, you're looking for our Dio integration, which does exactly what you're looking for when using Dio. If you use a different HTTP library, we'd be interested to know. For context - SentryHttpClient SentryClient (see below) is our SDK-internal HTTP client that performs the necessary HTTP requests from the SDK to Sentry and not something to be used by the host app.

ueman commented 1 month ago

@kahest I think you're confusing the SentryClient and the SentryHttpClient, since SentryHttpClient is the same thing as the DIO integration but for the http package.

To get the most out of the http integrations, you have to set the following options:

These options work for both, the SentryHttpClient and the sentry_dio integration.

Please be aware, that this is essentially a privacy and security nightmare, since you would be able to see users PII and their username/password. Please also be aware, that this only works when the request actually fails.

kahest commented 1 month ago

@ueman ah yes, you're totally right, thanks :)

sumanthratna commented 1 month ago

To get the most out of the http integrations, you have to set the following options:

These options work for both, the SentryHttpClient and the sentry_dio integration.

Please be aware, that this is essentially a privacy and security nightmare, since you would be able to see users PII and their username/password. Please also be aware, that this only works when the request actually fails.

Thanks! I set these options and I now see Request Body and Request Headers under my Sentry Issues. However, I don't see Response body. I skimmed through the Dart SDK source code and it looks like maxResponseBodySize is only used by the Dio integration. Does maxResponseBodySize work with SentryHttpClient?

edit: ~also, is there any mechanism to capture response headers, in the same way that SentryHttpClient captures request headers, request body, and response body?~ it seems like response headers are presented under Contexts

edit: shouldn't data be passed to SentryResponse if sendDefaultPii is true? (see below)

https://github.com/getsentry/sentry-dart/blob/178baee29ede5676069bf26c4228b37e1df7fdf3/dart/lib/src/http_client/failed_request_client.dart#L212-L216

sumanthratna commented 1 month ago

@kahest @ueman re: maxResponseBodySize, see https://github.com/getsentry/sentry-dart/issues/624 which proposes this feature and https://github.com/getsentry/sentry-dart/pull/1557 which implements it, only for Dio. I couldn't find a corresponding PR for the http integration.

Is my understanding correct that the Sentry Dart SDK should be updated to implement maxResponseBodySize for the http integration?

ueman commented 1 month ago

Yeah, seems like you're correct.

A note for the future implementer: Reading the body of a StreamedRequestfrom the http package hinders the body from being read again, meaning you need to deep clone it or something before returning it, in order to implement this feature. It's quite tricky to do correctly, so please be sure it works correctly before releasing it.