Skip to content
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

Jump to bottom

feat: Add ClientOptions.SendDefaultPii #485

Merged
merged 3 commits into from Nov 2, 2022
Merged

feat: Add ClientOptions.SendDefaultPii #485

merged 3 commits into from Nov 2, 2022

Conversation

cleptric
Copy link
Member

@cleptric cleptric commented Oct 30, 2022 •
edited

As scope.SetUser has to be explicitly called by the user, I did not add a check for SendDefaultPii there.

Closes #175

@cleptric cleptric marked this pull request as ready for review October 30, 2022 03:03
@cleptric cleptric self-assigned this Oct 30, 2022
@cleptric cleptric requested review from kamilogorek, AbhiPrasad and phacops and removed request for kamilogorek October 30, 2022 03:04
@cleptric
Copy link
Member Author

  • I've noticed that the PHP SDK still sends all headers but sets their value to [Filtered].
    Other SDKs drop the keys completely.
  • Do I need to add checks for the request body as well?

@AbhiPrasad
Copy link
Member

Do I need to add checks for the request body as well?

Yes, it's a source of PII as well

@cleptric
Copy link
Member Author

cleptric commented Nov 1, 2022

While the Request struct contains a Data field, I can't find any usage of this field in the SDK. So we're good to go with this PR.

@cleptric cleptric enabled auto-merge (squash) November 2, 2022 09:11
@cleptric cleptric added this to the v0.15.0 milestone Nov 2, 2022
@cleptric cleptric merged commit 3bb8296 into master Nov 2, 2022
@cleptric cleptric deleted the send-default-pii branch November 2, 2022 09:14
rhcarvalho
rhcarvalho reviewed Nov 2, 2022
View reviewed changes
CHANGELOG.md
- fix: Scope values should not override Event values (#446)
- feat: Extend User inteface by adding Data, Name and Segment (#483)
- feat: Make maximum amount of spans configurable (#460)
- feat: Add ClientOptions.SendDefaultPii #485
Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to see the Sentry Go SDK evolving!

One suggestion: initialisms like "PII" are spelled with a consistent case as per Go community conventions https://github.com/golang/go/wiki/CodeReviewComments#initialisms.

The option could be called ClientOptions.SendDefaultPII.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I opened #490

@cleptric cleptric mentioned this pull request Nov 2, 2022
Merged
@ikorihn ikorihn mentioned this pull request Jan 9, 2023
Closed
@tonyo tonyo mentioned this pull request Jun 1, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhcarvalho rhcarvalho rhcarvalho left review comments

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

@kamilogorek kamilogorek Awaiting requested review from kamilogorek

@phacops phacops Awaiting requested review from phacops

Assignees

@cleptric cleptric

Labels
Type: Feature
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.

PII opt-in by default
3 participants
@cleptric @AbhiPrasad @rhcarvalho