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(deps): Bump sentry-relay to v0.8.48 #66401

Merged
merged 3 commits into from Mar 6, 2024

Conversation

iker-barriocanal
Copy link
Member

Along the way of bumping dependencies, some tests are fixed to cover for the changes in Relay.

@iker-barriocanal iker-barriocanal self-assigned this Mar 6, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 6, 2024
Comment on lines 213 to 216
user = self.get_interface("user")
if user is not None:
user._data.pop("sentry_user", None)
return user
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes the test tests/sentry/eventstore/test_models.py::EventTest::test_snuba_data.

get_minimal_user intends to extract the minimal user that may be exposed to external services, so there's no reason to expose sentry_user. Alternatively, we could expose the value from Snuba, but I see no reason to do that as the field was originally added for DDM.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed, let's not change this function and fix the test assertion to compare a fixed set of fields instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 071238f.

@@ -122,7 +122,7 @@ def record_event_processed(project, event, **kwargs):
# Check to make sure more the ip address is being sent.
# testing for this in test_no_user_tracking_for_ip_address_only
# list(d.keys()) pattern is to make this python3 safe
if user_context and list(user_context.keys()) != ["ip_address"]:
if user_context and list(user_context.keys()) != ["ip_address", "sentry_user"]:
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes the test test_no_user_tracking_for_ip_address_only.

Relay now extracts sentry_user automatically from the IP address, and both should be filtered out together and not trigger user tracking.

@@ -42,7 +42,7 @@ def test_interface_is_relabeled():
manager.normalize()
data = manager.get_data()

assert data["user"] == {"id": "1"}
assert data["user"] == {"id": "1", "sentry_user": "id:1"}
Copy link
Member Author

Choose a reason for hiding this comment

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

sentry_user is now automatically added in Relay.

source: tests/sentry/event_manager/interfaces/test_spans.py
---
errors: null
to_json:
- data:
http.response.status_code: 200
Copy link
Member Author

Choose a reason for hiding this comment

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

Relay renamed the value for this key. It seems it didn't have a negative impact in other places. (same for the snapshot below)

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.29%. Comparing base (fa73383) to head (071238f).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #66401   +/-   ##
=======================================
  Coverage   84.29%   84.29%           
=======================================
  Files        5309     5309           
  Lines      237222   237222           
  Branches    41038    41038           
=======================================
+ Hits       199974   199975    +1     
+ Misses      37029    37028    -1     
  Partials      219      219           
Files Coverage Δ
src/sentry/receivers/features.py 90.82% <100.00%> (ø)

... and 3 files with indirect coverage changes

@iker-barriocanal iker-barriocanal marked this pull request as ready for review March 6, 2024 12:11
@iker-barriocanal iker-barriocanal requested review from a team as code owners March 6, 2024 12:11
@iker-barriocanal iker-barriocanal requested a review from a team March 6, 2024 12:11
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Pre-approving, please apply changes as discussed.

Comment on lines 213 to 216
user = self.get_interface("user")
if user is not None:
user._data.pop("sentry_user", None)
return user
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, let's not change this function and fix the test assertion to compare a fixed set of fields instead.

src/sentry/receivers/features.py Outdated Show resolved Hide resolved
src/sentry/eventstore/models.py Outdated Show resolved Hide resolved
src/sentry/receivers/features.py Outdated Show resolved Hide resolved
@iker-barriocanal iker-barriocanal merged commit 9898dee into master Mar 6, 2024
51 checks passed
@iker-barriocanal iker-barriocanal deleted the iker/ref/librelay-bump branch March 6, 2024 13:35
aliu3ntry pushed a commit that referenced this pull request Mar 6, 2024
Along the way of bumping dependencies, some tests are fixed to cover for
the changes in Relay.
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants