Skip to content
This repository has been archived by the owner on Aug 14, 2024. It is now read-only.

Spec HTTP client errors #711

Merged
merged 15 commits into from
Oct 19, 2022
Merged

Spec HTTP client errors #711

merged 15 commits into from
Oct 19, 2022

Conversation

marandaneto
Copy link
Contributor

Draft PR for this feature on Android - OkHttp getsentry/sentry-java#2287

@vercel
Copy link

vercel bot commented Oct 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
develop ✅ Ready (Inspect) Visit Preview Oct 19, 2022 at 6:59AM (UTC)

@romtsn
Copy link
Member

romtsn commented Oct 13, 2022

I noticed in the Android PR when dealing with headers you're also checking for sendDefaultPii + filtering out sensitive headers, should we mention it here in the spec as well? Otherwise LGTM, some minor wording things 👍

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
@marandaneto
Copy link
Contributor Author

I noticed in the Android PR when dealing with headers you're also checking for sendDefaultPii + filtering out sensitive headers, should we mention it here in the spec as well? Otherwise LGTM, some minor wording things 👍

Nope, reason is here getsentry/sentry-java#2287
I just aligned the behavior with the Java repo, but other SDKs don't guard the headers with defaultPii.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for writing down the spec and driving this new feature, @marandaneto. I have a couple of NITs and a few questions.

marandaneto and others added 2 commits October 17, 2022 11:28
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

One note about the range, apart from that great job @marandaneto 👏

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Sorry I completely forgot to ask: How should these errors be grouped together?

@marandaneto marandaneto marked this pull request as ready for review October 19, 2022 06:57
@marandaneto marandaneto enabled auto-merge (squash) October 19, 2022 06:57
@marandaneto marandaneto merged commit 277e2a1 into master Oct 19, 2022
@marandaneto marandaneto deleted the chore/http-client-errors branch October 19, 2022 07:00
@marandaneto
Copy link
Contributor Author

Sorry I completely forgot to ask: How should these errors be grouped together?

Stack trace normally, unless we see that this is not a good idea and can be improved later, but that's how Sentry works today, there's no special strategy unless you set fingerprints which can be done on eg beforeSend.

@philipphofmann
Copy link
Member

Thanks, @marandaneto, for being patient with all my questions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants