feat(spans): Convert a Sentry span to the Snuba span schema #2917
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jjbayer
requested changes
Jan 8, 2024
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
…t-from-protocol-to-snuba-spans-schema' into pierre/spans-convert-from-protocol-to-snuba-spans-schema
jjbayer
reviewed
Jan 8, 2024
jjbayer
reviewed
Jan 9, 2024
| ) # endpoint might overtake envelope | ||
|
|
||
| assert spans == [ | ||
| { | ||
| "organization_id": 1, |
There was a problem hiding this comment.
I think we still need the organization ID, right?
There was a problem hiding this comment.
No, we don't need organization_id anymore.
There was a problem hiding this comment.
Even if the field is described in the schema, the Snuba consumer doesn't need it to insert data: https://github.com/getsentry/snuba/blob/master/rust_snuba/src/processors/spans.rs#L28-L53.
jjbayer
approved these changes
Jan 9, 2024
There was a problem hiding this comment.
See comments about RawValue, but no blockers!
This definitely needs a Canary deploy, that is
- Pause
deploy-relay - Deploy
deploy-relay-experimental - Let run for ~1h and watch for consumer errors.
- If no errors and span volume is the same as before, unpause
deploy-relayand deploy there as well.
Canary can also be reverted quickly.
| ) # endpoint might overtake envelope | ||
|
|
||
| assert spans == [ | ||
| { | ||
| "organization_id": 1, |
…t-from-protocol-to-snuba-spans-schema' into pierre/spans-convert-from-protocol-to-snuba-spans-schema
phacops
deleted the
pierre/spans-convert-from-protocol-to-snuba-spans-schema
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're working on pushing spans directly to Snuba instead of going through a Sentry consumer. This will produce to Kafka the proper schema understood by the Snuba consumer.
The way I did that is a bit convoluted right now. The envelope contains a Sentry span we parse using the Span protocol which needs to then be converted to the Kafka message which will use serde. This can be simplified by parsing the span in the envelope directly into a serde-compatible struct (which means when we modify the span, we now have to modify 2 places). Do you think the trade-off is worth it?
I would have reused the annotated protocol but we then can't pass it nicely to the producer to be serialized.
It will still push to the
ingest-spanstopic until we change the config so we're going to need this PR merged and deployed before we can deploy this change: getsentry/sentry#62527.