feat(spans): Add a new recombiner consumer #66804
Conversation
shruthilayaj
changed the title
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #66804 +/- ##
==========================================
+ Coverage 79.36% 84.34% +4.97%
==========================================
Files 6364 5315 -1049
Lines 281973 237410 -44563
Branches 48521 41061 -7460
==========================================
- Hits 223788 200241 -23547
+ Misses 57822 36951 -20871
+ Partials 363 218 -145
|
|
Ah I need to define a schema for the new topic |
|
Blocked by getsentry/sentry-kafka-schemas#237 |
| @@ -45,6 +45,7 @@ class Topic(Enum): | |||
| GROUP_ATTRIBUTES = "group-attributes" | |||
| SHARED_RESOURCES_USAGE = "shared-resources-usage" | |||
| SNUBA_SPANS = "snuba-spans" | |||
| BUFFERED_SEGMENT = "buffered-segment" | |||
There was a problem hiding this comment.
Does this topic actually exist in prod?
There was a problem hiding this comment.
No, it does not. None of these consumers/topics/redis existing in prod yet
There was a problem hiding this comment.
Is there a concern of deploying this when the infrastructure doesn't exist?
There was a problem hiding this comment.
The consumer code is for sure okay to deploy without the infra in place (since ops actually needs to manually deploy the consumers), and the consumers producing to and reading from this topic aren't actually running in production, so I assumed it would be fine. @phacops do you know if that's the case?
There was a problem hiding this comment.
@evanh I'm going to make an ops ticket to get this topic configured and make sure that goes through before I merge this PR just to be safe
There was a problem hiding this comment.
The preference is to merge all the code first and then deploy infra to prod in the right order (details in the ticket), so I'm going to merge this PR cc: @evanh
| try: | ||
| process_segment(segments) | ||
| except Exception as e: | ||
| if random.random() < 0.05: |
There was a problem hiding this comment.
Updated to log exception instead - had just copied over an old pattern
src/sentry/utils/sdk.py
Outdated
| "/sentry/spans/consumers/process/factory.py", | ||
| "/sentry/spans/consumers/recombine/factory.py", |
There was a problem hiding this comment.
Why do those have a / prefixed in contrast to the others?
| "profile_id": "7ce060d7ea62432b8355bc9e612676e4", | ||
| "project_id": 1, | ||
| "received": 1706734067.029479, | ||
| "description": "OrganizationNPlusOne", |
There was a problem hiding this comment.
Just plopped in a different span here, sorry!
We're moving away from celery tasks + rabbit and instead
want a second consumer that processes segments. This
PR adds the new topic and consumer and refactors code
away from tasks.
This code doesn't run in prod yet.
In a follow up, the
process-spansconsumer will fetchsegments that are ready to be processed and push them
to the
buffered-segmentstopic