-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add protobuf support for onlyspans ingestion #3044
Conversation
a7dd2fd
to
d71949c
Compare
3f10db7
to
f8a5997
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me, leaving the final review to @jjbayer as I don't have enough context to provide proper feedback.
a525fa1
to
1f9b731
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely have to make sure to not have an unpinned git dependency. Also would like to get a review from @jjbayer, I am missing context and was just looking for Rust stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sl0thentr0py thanks for doing this! Nice to see some LoCs removed. Please add a test for binary protobuf ingestion to test_store.py
. See
relay/tests/integration/test_store.py
Line 1412 in 1f9b731
def test_span_ingestion( |
1f9b731
to
2c9d457
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks looks good to me! Left a small improvement to the handle
function.
It would also be nice to have at least one integration test which makes sure we're parsing the data correctly.
the date conversion helper from strings is not added to their serde derives, so I've ignored this test for now
To you still want to address this todo from your PR description?
* use official `prost` and `serde` compatible structs from `opentelemetry-proto` * use `axum-extra` with the `protobuf` feature for the protobuf extractor
b0c0351
to
423f82a
Compare
added integration test |
423f82a
to
117a2a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
prost
andserde
compatible structs fromopentelemetry-proto
axum-extra
with theprotobuf
feature for the protobuf extractorTODO
trace_id/span_id
areVec<u8>
, fix that conversionthe date conversion helper from strings is not added to their serde derives, so I've ignored this test for nowwill need to do upstream later