feat(occurrences): Add RPCBase boilerplate#104842
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
thetruecpaul
force-pushed
the
cpaul/eap/add-occurrences-rpc
branch
from
5ba15f7 to
8ddd2e1
Compare
thetruecpaul
force-pushed
the
cpaul/eap/add-occurrences-rpc
branch
from
8ddd2e1 to
673d536
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| orderby: list[str] | None, | ||
| offset: int, | ||
| limit: int, | ||
| referrer: str, | ||
| config: SearchResolverConfig, | ||
| sampling_mode: SAMPLING_MODES | None = None, | ||
| equations: list[str] | None = None, | ||
| page_token: PageToken | None = None, | ||
| additional_queries: AdditionalQueries | None = None, | ||
| debug: bool = False, | ||
| ) -> EAPResponse: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
shashjar
left a comment
shashjar
left a comment
There was a problem hiding this comment.
overall looks good, thanks. left some nits + request for tests
|
|
||
| @classmethod | ||
| @sentry_sdk.trace | ||
| def run_table_query( |
There was a problem hiding this comment.
Same as bot comments - seems like we can put debug on the SnubaParams instead of a separate arg, and looks like additional_queries aren't passed through
There was a problem hiding this comment.
Same for run_table_query_with_tags?
src/sentry/snuba/occurrences_rpc.py
Outdated
| tags should be formatted appropriately - e.g. {tags[foo], tags[bar]} | ||
| """ | ||
|
|
||
| columns = OccurrencesRPC.DEFINITIONS.columns.copy() |
There was a problem hiding this comment.
nit: can use cls.DEFINITIONS here
| logger = logging.getLogger("sentry.snuba.occurrences_rpc") | ||
|
|
||
|
|
||
| class OccurrencesRPC(rpc_dataset_common.RPCBase): |
There was a problem hiding this comment.
Can we add tests for this? I think we can extend the parametrized tests in tests/sentry/snuba/test_rpc_dataset_common.py. I also see there are tests for search resolution for the other datasets (e.g. tests/sentry/search/eap/test_spans.py) - not sure if we've implemented everything we need in order to add those tests for occurrences, but if so can we do that?
thetruecpaul
force-pushed
the
cpaul/eap/add-occurrences-rpc
branch
from
673d536 to
146af52
Compare
| ), | ||
| ] | ||
| ) | ||
| } |
There was a problem hiding this comment.
Bug: Missing trace column definition in occurrence columns
OCCURRENCE_COLUMNS is missing the trace column definition that all other RPC implementations (Spans, OurLogs, ProfileFunctions) explicitly include. The base class RPCBase.get_table_rpc_request and get_timeseries_query call resolver.resolve_column("trace") to handle trace-based sampling optimizations. Without the explicit definition, the resolver falls back to creating a dynamic ResolvedAttribute with internal_name="trace" instead of the expected "sentry.trace_id". This causes trace-based queries to use the wrong internal column name, which would result in failed or incorrect query results against the Snuba backend.
| public_alias=tag_name, | ||
| internal_name=tag_name, | ||
| search_type="string", | ||
| ) |
There was a problem hiding this comment.
Bug: Incorrect internal_name format for tag columns
In run_table_query_with_tags, the internal_name for tag columns is incorrectly set to tag_name (e.g., "tags[foo]") instead of just name (e.g., "foo"). When the SearchResolver dynamically resolves a tag like tags[foo], it extracts just "foo" as the internal_name using regex. This inconsistency means pre-registered tag columns would have a different internal_name format than dynamically resolved ones, causing the RPC AttributeKey.name to be set incorrectly and potentially causing queries to fail or return wrong results.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #104842 +/- ##
========================================
Coverage 80.45% 80.46%
========================================
Files 9397 9399 +2
Lines 403608 403694 +86
Branches 25925 25925
========================================
+ Hits 324730 324816 +86
Misses 78438 78438
Partials 440 440 |
There's a lot of good work & validation in this framework. Let's pick it up and use it for our own ends. This PR adds an implementation of RPCBase for use with Occurrence trace items.
thetruecpaul
force-pushed
the
cpaul/eap/add-occurrences-rpc
branch
from
146af52 to
e2f7b67
Compare
2 participants
There's a lot of good work & validation in this framework. Let's pick it up and use it for our own ends.
This PR adds an implementation of RPCBase for use with Occurrence trace items.