Request and Response bodies for GraphQL errors #93

marandaneto · 2023-05-17T10:58:31Z

Describe your RFC briefly so that reviewers can quickly find out what
happens. Also please edit the "Rendered RFC" link so one can quickly
get to the rendered markdown file.

Rendered RFC

text/0087-graphql-errors.md

marandaneto · 2023-05-17T11:08:30Z

text/0087-graphql-errors.md

+
+- Do we need to send the GraphQL scheme to Sentry in order to do data scrubbing properly?
+- Should this feature be opt-in due to the size of the data and PII risk?
+- Should we send the Request and Response as different envelope items? (avoid the size limit)


See option 3 and 4.

marandaneto · 2023-05-17T11:09:04Z

text/0087-graphql-errors.md

+
+# Unresolved questions
+
+- Do we need to send the GraphQL scheme to Sentry in order to do data scrubbing properly?


It'd require changes on sentry-cli, data scrubbing, etc.

Probably not, because the PII will be under variables values per spec. and we can use that to match and strip out the PII data.

marandaneto · 2023-05-17T11:09:38Z

text/0087-graphql-errors.md

+
+SDKs should discard large and binary bodies by default, using the [maxRequestBodySize](https://docs.sentry.io/platforms/android/configuration/options/#max-request-body-size) and `maxResponseBodySize` (it'll be added) options.
+
+The difference is that for GraphQL errors, this should be enabled by default.


If we don't send the payload, the user won't have context about what has to be fixed.

The other option is to manipulate the request and payload sent, for example, sending only the errors of the response and the body without the variables from the request, which requires serializing/deserializing the request and response of the client, this might add performance overhead as just sending it as it is, but it's an option.

How will we handle situations where partial data are returned with errors? Will we only send the errors to Sentry or the filtered data too?

It's still an error, https://www.notion.so/sentry/GraphQL-proposal-d6e5846f30434770903cf3af20bc2568?pvs=4#65c51cb28c1c421e8b3edd4f5fe2f427
I'll check during the implementation if there's a performance impact of serializing the error, extracting the non-required fields, etc.

marandaneto · 2023-05-17T11:33:15Z

@scefali @malwilley adding you to the loop since this change affects the FE for syntax highlight.

cleptric · 2023-05-17T11:33:45Z

text/0087-graphql-errors.md

+
+## Must have
+
+The fields `Request#data` and `Response#data` could contain PII and they should run data scrubbing agressively.


We should consider allowing people to opt out of scrubbing. Often, the actual payload is the information needed to reproduce bugs.

Data scrubbing rules are already possible in the project config, but not everything, we'd stick with what's in there.

Which project config? Are you talking about server-side scrubbing?

Yes, eg https://sentry-sdks.sentry.io/settings/projects/sentry-cocoa/security-and-privacy/
Scrubbing will be server-side, so if there's an option to enable/disable, it should be there.
People can also opt-in for SDK scrubbing using beforeSend.

text/0087-graphql-errors.md

marandaneto · 2023-05-17T12:12:35Z

The event will likely need an identification either via mechanism.type or something else to trigger Relay scrubbing specific to GraphQL spec.
WDYT @getsentry/ingest ?

text/0087-graphql-errors.md

philipphofmann

I'm a bit confused by this RFC 🤔. The title is Request and Response bodies for GraphQL errors, the summary describes

Add Request and Response body to events.

and the option 1 is about

adding a data field in the [Response]

I'm totally on board with option 1 to solve the problem for GraphQL errors. What I'm unsure about is if we also want to decide in this RFC

if we should add request and response bodies to events (that's unclear to me)
how to send GraphQL request and response data (I would assume we don't want to decide that in this PR)

Could you please help me 😄, @marandaneto?

text/0087-graphql-errors.md

philipphofmann · 2023-05-22T09:48:28Z

text/0087-graphql-errors.md

+
+[Session Replay](https://docs.sentry.io/platforms/javascript/guides/remix/session-replay/configuration/) already sends the request and response bodies, so we can use the same data scrubbing rules.
+
+Since GraphQL is a well defined spec, we can also scrub the GraphQL fields.


What about only sending the fields we are sure about not containing any PII? Then we could turn this feature on by default and no scrubbing would be needed.

The downside is the performance impact of serializing on the client and duplicating all the data scrubbing logic in every SDK, hence focusing on Relay for PII.
It's also easier to roll out bug fixing on Relay.
It is a valid option though.

I'll add it as a note.

Edit: it's part of the Unresolved questions already:

Should PII be scrubbed in the SDK instead?

Doing it on every SDK is a downside, yes, but the logic for only selecting well-known fields shouldn't be that complicated. We could also move it to Relay if it is too complicated.

During the initial discussion, we opted for Relay to avoid duplication and an easy way of rolling out bug fixes as described in the message above, if it turns out to be super simple, and has no performance impact at all, we can evaluate it in the next iteration.
Unless there's a strong reason otherwise, Relay still should do PII on its own anyway.

marandaneto · 2023-05-22T10:49:36Z

I'm a bit confused by this RFC 🤔. The title is Request and Response bodies for GraphQL errors, the summary describes

Add Request and Response body to events.

and the option 1 is about

adding a data field in the [Response]

I'm totally on board with option 1 to solve the problem for GraphQL errors. What I'm unsure about is if we also want to decide in this RFC

if we should add request and response bodies to events (that's unclear to me)

how to send GraphQL request and response data (I would assume we don't want to decide that in this PR)

Could you please help me 😄, @marandaneto?

@philipphofmann

if we should add request and response bodies to events (that's unclear to me)

Sending the request and response bodies to Sentry isn't optional, this is needed for the user to know what's the issue about, how to fix it, and have syntax highlight as well, so it's either doing this or not implementing this feature.
See the background section:

Because of that, the Request and Response bodies should be sent to Sentry.

how to send GraphQL request and response data (I would assume we don't want to decide that in this PR)

The goal as of now is to just send it as part of the request.data and response.data, and Relay will do its work.
Those events are only going to be raised if the response.data actually has the errors field in it, I'll address this more in detail on https://develop.sentry.dev/ later on with the feature spec.
The goal here is to get request and response bodies in Sentry, but with a sensible reason and background.

text/0087-graphql-errors.md

Co-authored-by: Roman Zavarnitsyn <[email protected]>

jjbayer · 2023-05-22T13:12:18Z

@marandaneto

The event will likely need an identification either via mechanism.type or something else to trigger Relay scrubbing specific to GraphQL spec.

In the RFC, you wrote

By doing this, we can keep the Request interface as it is, we can copy the data scrubbing rules from the Request interface.

I am confused now, do we want to apply the same data scrubbing rules as for request.data, or do we need to introduce new scrubbing rules? In the latter case, we would need to define a strict protocol for graphql payloads.

text/0087-graphql-errors.md

jjbayer · 2023-05-22T13:06:04Z

text/0087-graphql-errors.md

+
+# Proposal (Option 1)
+
+The proposal is adding a `data` field in the [Response](https://develop.sentry.dev/sdk/event-payloads/types/#responsecontext) interface.


Dart seems to use the key body instead of data: https://github.com/getsentry/sentry-dart/blob/7011abe27ac69bd160bdc6ecf3314974b8340b97/dart/lib/src/protocol/sentry_response.dart#L59

I think data is better because it is consistent with Request, but I wanted to point out the inconsistency.

Good point, I can deprecate body and add data, both will serialize as data anyway.

Co-authored-by: Joris Bayer <[email protected]>

marandaneto · 2023-05-22T14:29:40Z

@marandaneto

The event will likely need an identification either via mechanism.type or something else to trigger Relay scrubbing specific to GraphQL spec.

In the RFC, you wrote

By doing this, we can keep the Request interface as it is, we can copy the data scrubbing rules from the Request interface.

I am confused now, do we want to apply the same data scrubbing rules as for request.data, or do we need to introduce new scrubbing rules? In the latter case, we would need to define a strict protocol for graphql payloads.

The goal is that if the event isn't a GraphQL error, it should run data scrubbing for response.data anyway similarly to request.data.
If the Event is a GraphQL error, then yes, we'll need to run data scrubbing based on the graphql protocol, so technically both things.
See the question #93 (comment)
We need a marker to make the difference between the normal scrubbing and the graphql protocol scrubbing.

marandaneto · 2023-05-23T09:34:56Z

@jjbayer and @jan-auer is there something you'd like to see changed in this RFC or are we good to go?

jjbayer · 2023-05-23T10:58:26Z

If the Event is a GraphQL error, then yes, we'll need to run data scrubbing based on the graphql protocol

@marandaneto so you want to parse the graphql and iterate over its keys / values, and apply existing data scrubbing rules to those parsed items? If so, we definitely need a discriminator. I would put it inside the request / response, maybe an optional type field in both interfaces?

marandaneto · 2023-05-23T11:13:23Z

If the Event is a GraphQL error, then yes, we'll need to run data scrubbing based on the graphql protocol

@marandaneto so you want to parse the graphql and iterate over its keys / values, and apply existing data scrubbing rules to those parsed items? If so, we definitely need a discriminator. I would put it inside the request / response, maybe an optional type field in both interfaces?

yes, that can be the mechanism as suggested here #93 (comment)
Or it could be somewhere else as well, no strong feelings about that, where it'd make more sense, not sure if it makes sense to be part of the request/response, though probably something under the event makes more sense.
The FE bits will need that too for the special casing of the issue view and syntax highlight.

philipphofmann

LGTM

marandaneto added 3 commits May 17, 2023 12:58

Request and Response bodies for GraphQL errors

875f3d0

pr id

196f43e

fix

c9a4022

marandaneto commented May 17, 2023

View reviewed changes

text/0087-graphql-errors.md Outdated Show resolved Hide resolved

fix

c4b3099

marandaneto commented May 17, 2023

View reviewed changes

text/0087-graphql-errors.md Show resolved Hide resolved

marandaneto commented May 17, 2023

View reviewed changes

fix

f838773

marandaneto requested review from scefali, malwilley and a team May 17, 2023 11:32

cleptric reviewed May 17, 2023

View reviewed changes

text/0087-graphql-errors.md Outdated Show resolved Hide resolved

cleptric reviewed May 17, 2023

View reviewed changes

text/0087-graphql-errors.md Outdated Show resolved Hide resolved

marandaneto added 3 commits May 17, 2023 14:07

fix

fec4b63

fix

4ea7a3c

fix

a52b77a

marandaneto and others added 2 commits May 17, 2023 14:18

fix

527e3b2

add graphql syntax

8ae39f8

krystofwoldrich reviewed May 22, 2023

View reviewed changes

text/0087-graphql-errors.md Show resolved Hide resolved

philipphofmann reviewed May 22, 2023

View reviewed changes

marandaneto added 2 commits May 22, 2023 12:38

fixes

cc8830a

fixes

ccc7791

romtsn reviewed May 22, 2023

View reviewed changes

text/0087-graphql-errors.md Outdated Show resolved Hide resolved

Update text/0087-graphql-errors.md

b0cf3f4

Co-authored-by: Roman Zavarnitsyn <[email protected]>

jjbayer reviewed May 22, 2023

View reviewed changes

Update text/0087-graphql-errors.md

2d811bb

Co-authored-by: Joris Bayer <[email protected]>

fix changelog

2484890

marandaneto marked this pull request as ready for review May 23, 2023 09:33

marandaneto requested review from jjbayer and jan-auer May 23, 2023 09:36

jjbayer approved these changes May 23, 2023

View reviewed changes

philipphofmann approved these changes May 24, 2023

View reviewed changes

Merge branch 'main' into choregraphql-errors

5b180eb

marandaneto merged commit b92c3bf into main May 30, 2023
1 check passed

marandaneto deleted the choregraphql-errors branch May 30, 2023 08:20

smeubank mentioned this pull request Aug 18, 2023

Feature request: GraphQL integration getsentry/sentry-dotnet#2449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request and Response bodies for GraphQL errors #93

Request and Response bodies for GraphQL errors #93

marandaneto commented May 17, 2023 •

edited

Loading

marandaneto May 17, 2023

marandaneto May 17, 2023

marandaneto May 17, 2023

marandaneto May 17, 2023

marandaneto May 17, 2023

krystofwoldrich May 22, 2023

marandaneto May 22, 2023

marandaneto commented May 17, 2023

cleptric May 17, 2023

marandaneto May 17, 2023

cleptric May 17, 2023

marandaneto May 17, 2023

marandaneto commented May 17, 2023

philipphofmann left a comment •

edited

Loading

philipphofmann May 22, 2023

marandaneto May 22, 2023

marandaneto May 22, 2023 •

edited

Loading

philipphofmann May 22, 2023

marandaneto May 22, 2023

marandaneto commented May 22, 2023

jjbayer commented May 22, 2023

jjbayer May 22, 2023

marandaneto May 22, 2023

marandaneto commented May 22, 2023

marandaneto commented May 23, 2023

jjbayer commented May 23, 2023

marandaneto commented May 23, 2023 •

edited

Loading

philipphofmann left a comment


		# Unresolved questions

		- Do we need to send the GraphQL scheme to Sentry in order to do data scrubbing properly?


		SDKs should discard large and binary bodies by default, using the [maxRequestBodySize](https://docs.sentry.io/platforms/android/configuration/options/#max-request-body-size) and `maxResponseBodySize` (it'll be added) options.

		The difference is that for GraphQL errors, this should be enabled by default.


		## Must have

		The fields `Request#data` and `Response#data` could contain PII and they should run data scrubbing agressively.


		[Session Replay](https://docs.sentry.io/platforms/javascript/guides/remix/session-replay/configuration/) already sends the request and response bodies, so we can use the same data scrubbing rules.

		Since GraphQL is a well defined spec, we can also scrub the GraphQL fields.


		# Proposal (Option 1)

		The proposal is adding a `data` field in the [Response](https://develop.sentry.dev/sdk/event-payloads/types/#responsecontext) interface.

Request and Response bodies for GraphQL errors #93

Request and Response bodies for GraphQL errors #93

Conversation

marandaneto commented May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented May 17, 2023

philipphofmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented May 22, 2023

jjbayer commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented May 22, 2023

marandaneto commented May 23, 2023

jjbayer commented May 23, 2023

marandaneto commented May 23, 2023 • edited Loading

philipphofmann left a comment

Choose a reason for hiding this comment

marandaneto commented May 17, 2023 •

edited

Loading

philipphofmann left a comment •

edited

Loading

marandaneto May 22, 2023 •

edited

Loading

marandaneto commented May 23, 2023 •

edited

Loading