Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add integration for Apollo-Kotlin 3 #2109
Add integration for Apollo-Kotlin 3 #2109
Changes from 1 commit
770b0c9
d2a6428
75f9086
2418613
e70c24a
a99e873
71dabc0
ea95a8c
e32dae4
b2112d7
683d374
6f2d6be
ab17056
700504e
6164c80
91a0b2d
d58842a
1eba47c
c750672
5f906ab
7d95756
65fdef8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this blow up if there's long variables? Do we need to truncate / skip if too long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point... Don't think it would blow up because header sizes are usually limited by the server side. However, in order to keep our request the Sentry Server small we should truncate/skip.
Is that something we could/should use
SentryOptions#maxRequestBodySize
for?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxRequestBodySize
is about the body only, headers are added as it is, IIRC this should be guarded bydefaultPii
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marandaneto Actually these headers are meant to be passed internally only in order to work around the need to deserialize.
They are added by the
SentryApollo3RequestComposer
and then read by theSentryApollo3HttpInterceptor
and then removed. Only if people use theRequestComposer
without the interceptor will the headers be sent over the wire. But then they don't get any use out of it as theInterceptor
is the one to send info to our Backend.I think Alex's question was about size limits for headers in general.
As for my idea with the
maxRequestBodySize
: Depending on the Apollo Operation the variables could get quite big and would therefore increase our payload sent to the Backend. Should we set a fixed sizeLimit or just send the variables as they are?We thought about guarding with
defaultPii
but decided against it, correct @adinauer ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just send the variables as they are.
We don't do any trimming on the client, the server drops if it exceeds and reports to the
client_reports
feature.They can use the filtering callbacks to drop stuff in case they need to.
@lbloder using the request's header for this workaround could be problematic, in case this is a request for a 3rd party and somebody isn't using the interceptor, we can be leaking some PII data, I guess?
If there's no way around getting everything we need either with both interceptors, I'd rather go with the option that has more useful data and open an issue on the
apollo3
repo with a feature request explaining our use case.Maybe @bruno-garcia has opinions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, are we talking about sending the
Request
andHeaders
to our Backend?Right now, we don't send the
Request
or itsHeaders
to our Backend in theInterceptors
, neither are we doing this in e.g.SentryOkHttpInterceptor
nor in theSentryFeignClient
if I'm not mistaken. @marandaneto You said this is in our spec? Does this apply to mobile as well? Should we open issues for the other integrations as well?If we do this, then Headers should most definitely be guarded behind PII imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just found your original comment, so please ignore the above.
As for my workaround with using headers to pass data between
Interceptors
, I don't see a problem if our headers are leaked, except that the request is getting bigger, because the headers we add only contain info that is in the request body anyways.In terms of sending the whole request to the server, then I would definitely do it like in the
SentryRequestResolver
that @adinauer linkedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not sending headers in this PR to Sentry, no need to worry, but if we do, we should guard against defaultPii, See https://github.com/getsentry/sentry-dart/blob/d75a8c698a4942816336f3bae9a50c87fa6be0a8/dio/lib/src/dio_event_processor.dart#L117
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it easier to use with an extension function that adds both interceptors. Also this should make it harder to misconfigure,
The only headers we would send, and only if the
SentryApollo3HttpInterceptor
isn't added, are those that containvariables
operationName
andoperationType
which are all present in the request body anyways.I would split sending the
request
and itsheaders
to a separate issue for the integrations that are not sending those yet. I.e. The Apollo Integration v2 and v3, the OkHttp Integration, the FeignClient and maybe others, will look through them and create issues for those that are not sending therequest
and itsheaders
to our backend.Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have #2148 to track.