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

ref(grouping): Add project_root to debug_meta in event #3941

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Jan 16, 2025

As part of the "Incorporate any internal logic from the SDK in the server-side grouping config" task from getsentry/sentry#83603, this sends the project_root value in the outgoing payload, under debug_meta. This will allow the sentry server to replicate the logic here.

Note to reviewers: If there's a better place to put it, please let me know. If debug_meta is a good spot, we'll need to also make a PR to Relay to add project_root as a recognized entry. (I have the change in a branch, but won't push it until we decide.)

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.16%. Comparing base (3f57299) to head (6caf359).
Report is 4 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3941      +/-   ##
==========================================
- Coverage   80.17%   80.16%   -0.01%     
==========================================
  Files         139      139              
  Lines       15394    15397       +3     
  Branches     2596     2597       +1     
==========================================
+ Hits        12342    12343       +1     
- Misses       2207     2209       +2     
  Partials      845      845              
Files with missing lines Coverage Δ
sentry_sdk/client.py 78.60% <100.00%> (+0.14%) ⬆️

... and 5 files with indirect coverage changes

@lobsterkatie lobsterkatie force-pushed the kmclb-send-project-root-in-event branch from caaf3cf to 6caf359 Compare January 17, 2025 01:28
@lobsterkatie lobsterkatie marked this pull request as ready for review January 17, 2025 07:44
@sentrivana
Copy link
Contributor

@getsentry/ingest Could you folks have a look if debug_meta is a good place to put project_root?

TL;DR: We are moving some of the in-app detection logic for stack frames from the SDK to the server, see getsentry/sentry#83603. In order to do this, the server needs to know the project_root set by the user in the SDK.

@lobsterkatie
Copy link
Member Author

lobsterkatie commented Jan 22, 2025

@getsentry/ingest Could you folks have a look if debug_meta is a good place to put project_root?

In my testing I've discovered that in order for this to work, I'm also going to have to update Relay. (There's a catch-all other entry in the debug_meta schema, so Relay won't reject project_root there, but during normalization we strip out any unrecognized entries.) I've updated the PR description above to reflect that, and will push said Relay PR if everything seems good to go.

UPDATE: I'm going to assume for now that debug_meta is a reasonable spot, so that I can get the next PR up. If the ingest team weighs in with a different proposed location, it's easy enough to thing to change. To that end, I've put up the relay PR here and the PR to use project_root on the server here.

@jjbayer
Copy link
Member

jjbayer commented Jan 27, 2025

  1. Since project_root is an SDK option, should we put it in event.sdk instead?
  2. IIUC project_root will contain PII, so we'll probably want to scrub at least the username in relay. And then the scrubbing rules for the stack frames have to match the scrubbing rules for project_root for server-side comparison to work. All in all, would it be easier to send a boolean flag in_project_root as part of the stack frame instead?

@lobsterkatie
Copy link
Member Author

  1. Since project_root is an SDK option, should we put it in event.sdk instead?

It's not really an SDK option, though. Yes, you can set it manually, but the more usual case is that the SDK just calls os.getcwd().

  1. IIUC project_root will contain PII, so we'll probably want to scrub at least the username in relay. And then the scrubbing rules for the stack frames have to match the scrubbing rules for project_root for server-side comparison to work. All in all, would it be easier to send a boolean flag in_project_root as part of the stack frame instead?

Do we actually scrub abs_path in the stacktrace? In any case, your other idea is an interesting one. Let me play around with it a bit.

@jjbayer
Copy link
Member

jjbayer commented Jan 28, 2025

Do we actually scrub abs_path in the stacktrace?

At least it can be scrubbed by Advanced Data Scrubbing rules.

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

Successfully merging this pull request may close these issues.

3 participants