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

feat(partnerships): backend service for partnership methods #87817

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

Conversation

sentaur-athena
Copy link
Member

@sentaur-athena sentaur-athena commented Mar 24, 2025

This PR is milestone 2 of this tech spec, Sentry side: https://www.notion.so/sentry/Nintendo-inbound-filter-1b58b10e4b5d80dc9090ec64d2825349?pvs=4#1ba8b10e4b5d802ba55fc0313f98f6cd

Getsenrty implementation: https://github.com/getsentry/getsentry/pull/17033
In the next PR I'll add it to project config code.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 24, 2025
Copy link

codecov bot commented Mar 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #87817    +/-   ##
========================================
  Coverage   87.73%   87.74%            
========================================
  Files        9925     9930     +5     
  Lines      563897   564240   +343     
  Branches    22208    22208            
========================================
+ Hits       494761   495077   +316     
- Misses      68734    68761    +27     
  Partials      402      402            

@Dav1dde
Copy link
Member

Dav1dde commented Mar 25, 2025

I'm not sure about the object format that would be useful to pass to relay

The format is typed here in Sentry:

class GenericFilter(TypedDict):
"""Configuration for a generic filter that filters incoming events."""
id: str
isEnabled: bool
condition: RuleCondition

Don't know how to test this

The filter itself is in the tech spec and added as a test in Relay. Actually testing if the filter works is currently not possible, librelay (Relay's Python Lib) only exposes a way to validate rule conditions (relay.validate_rule_condition).

I think testing that the filter is added to the config should be sufficient. But it may make sense to add the filter in a separate PR and then hide it behind a feature flag/options automator option to control the rollout.

@markstory
Copy link
Member

Don't know how to test this

There isn't much to test yet. I think the tests will come with the getsentry implementation. When you are connecting Partnership to project configuration, you could use a stub/mock implementation that returns placeholder filter rules to validate that any rules returned by Partnership are included in the project's ruleset.

Copy link
Member

@isabellaenriquez isabellaenriquez left a comment

Choose a reason for hiding this comment

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

what david said (testing that the filter is added in the expected cases) makes sense and that your tests right now are sufficient for what we currently have

@sentaur-athena
Copy link
Member Author

So please go ahead and review this and getsentry side of it. After merging these I add it to the project config code and add the tests there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants