feat: Send PII to Spotlight when no DSN is set

[BYK]

Quick fix for getsentry/spotlight#543 until we implement a global scrubber that only scrubs events sent to the clound thorugh the DSN.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.90%. Comparing base (295dd8d) to head (e45e581).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3804      +/-   ##
==========================================
- Coverage   79.90%   79.90%   -0.01%     
==========================================
  Files         137      137              
  Lines       15382    15385       +3     
  Branches     2610     2611       +1     
==========================================
+ Hits        12291    12293       +2     
  Misses       2220     2220              
- Partials      871      872       +1     

Files with missing lines	Coverage Δ
sentry_sdk/client.py	78.45% <100.00%> (+0.14%)	⬆️
sentry_sdk/consts.py	93.21% <ø> (ø)

... and 1 file with indirect coverage changes

[antonpirker]

I made the initialization of our EventScrubber more explicit. It did work before, but now it is "more correct" in a way.

[antonpirker]

I think this change is fine. Changing the default value of send_default_pii from False to None in ClientConstructor is kind of a sensitive change, but as we use should_send_default_pii() everywhere, this is fine.

not sure.

[antonpirker]

I have now refactored your change @BYK to not change the default value of send_default_pii. It does the same, but I feel better about it this way.

If you are ok with this, I will merge it! (or you can merge it, to avoid another round trip)

[antonpirker]

Now I feel good about this.

[BYK]

@antonpirker I disagree with this direction and also with the suggestion that changing the default is a bad idea. The default, by definition, is a selection we make on behalf of the user when they have no preference. This means, if they explicitly set it to False we should honor that even for Spotlight.

Keeping it this way takes away that option, meaning taking away control.

What is the motivation you had for the refactoring and added changes. Is it only type purity?

[antonpirker]

Agreed. Maybe I am just paranoid. I reverted my refactoring. Changing send_default_pii is fine.

[antonpirker]

After some discussion, I am convinced that this is fine.

[BYK]

Agreed. Maybe I am just paranoid.

I don't think you were being paranoid. Since we have not communicated what was the concern on the PR itself, I'm just recording it here for posterity:

@antonpirker's concern was some code potentially reading send_default_pii value and checking it against False explicitly to make decisions. I think this was a valid concern but we audited the code and nothing seems to be doing that. Also, our long term plan is to change the logic around PII and instead of checking this value at every point of collection, use it right before sending the events upstream to a DSN along with "PII fields" and a scrubber that removes those fields if send_default_pii is set to False. This would allow us to send unfiltered events to Spotlight without messing with these settings and changing them for the upstream case.

[antonpirker]

@BYK A note about stripping PII at the point of collection (in the integration) vs in a central place right before sending:

The benefit of stripping in the integration is, that the integration knows best what data could contain PII and what data does not. Doing this in a central place could lead a lot of "if span is from mongodb: strip that field. if span is from anthropic AI: strop those fields". This can get messy fast.

[BYK]

@antonpirker ah, I missed one crucial detail (or failed to explain it properly). The core idea here is integrations still announcing/marking those fields as sensitive in a central place where the scrubber just goes through the mundane job of removing those fields from the final payload.

Sounds better? :)

[antonpirker]

Sounds better, indeed! :-)