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: Implement Envelope::from_path and Envelope::from_slice #456

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Apr 14, 2022

The implementation is highly inspired by https://github.com/getsentry/relay/blob/master/relay-server/src/envelope.rs and it includes most its deserialization tests.

We do not handle unknown item types for now, as it would require significant changes in how serializing works and how we store the envelopes. Thus we decided to skip it, for now, to support only item types that sentry-rust implements and enhance this feature in the future.

This will be enough to unblock getsentry/sentry-cli#703 anywya.
Closes #381

@codecov-commenter
Copy link

Codecov Report

Merging #456 (8ba95a0) into master (799f211) will increase coverage by 0.22%.
The diff coverage is 92.50%.

❗ Current head 8ba95a0 differs from pull request most recent head 7f883b3. Consider uploading reports for the commit 7f883b3 to get more accurate results

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
+ Coverage   80.43%   80.66%   +0.22%     
==========================================
  Files          73       73              
  Lines        8364     8441      +77     
==========================================
+ Hits         6728     6809      +81     
+ Misses       1636     1632       -4     

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I’m a bit surprised any of this works, lol. Probably because it just skips all the item headers as something that is not parsable, and then parses the item payload itself. That in turn means you will have problems parsing an Attachment item, which I see is also missing a test, probably for that reason ;-)

Instead of using serde magic, it might be a better idea to manually parse/match the item headers. Might as well create a derive(Deserialize) ItemHeader type, which has at least a type field, and possibly a length as well. In order to support attachments, you would have to make your parser more sophisticated as well, as going by lines() will break for attachments.

@kamilogorek kamilogorek changed the title feat: Implement Envelope::from_path and Envelope::from_reader feat: Implement Envelope::from_path and Envelope::from_slice Jun 17, 2022
sentry-types/src/protocol/attachment.rs Outdated Show resolved Hide resolved
}

#[derive(Deserialize)]
struct EnvelopeHeader {
Copy link
Member

Choose a reason for hiding this comment

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

Should we care about more optional attributes like sent_at, dsn, etc? Probably not now, since envelope "round-trips" are not really a goal for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need to change to_writer as well to handle this, as we currently ignore anything else other than event_id.

}

/// An Envelope Item Header.
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[allow(dead_code)]

We are using this, maybe we are not exposing the transitive users publicly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was due to unused attachment_type. Fixed it by using the value from item header and implementing FromStr for an AttachmentType

sentry-types/src/protocol/envelope.rs Outdated Show resolved Hide resolved
fn parse_items(slice: &[u8], mut offset: usize) -> Result<Vec<EnvelopeItem>, EnvelopeError> {
let mut items = Vec::new();

while offset < slice.len() {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having a mut offset and sub-slicing from the original slice all the time, we can just have mut slice and do a slice = slice.get(offset..)? after we read each item, and above in from_slice.
But it doesn’t really matter that much anyway.

sentry-types/src/protocol/envelope.rs Outdated Show resolved Hide resolved

let item = match header.r#type {
EnvelopeItemType::Event => EnvelopeItem::Event(
serde_json::from_slice(payload).map_err(EnvelopeError::InvalidItemPayload)?,
Copy link
Member

Choose a reason for hiding this comment

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

This duplicated map_err is a bit annoying.

We could potentially make this serde_json::from_slice(payload).map(EnvelopeItem::Event), and an explicit Ok(EnvelopeItem::Attachment(…)) below.
That way we can move the .map_err(…)? outside the match. That can potentially improve readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So much better now :)

sentry-types/src/protocol/envelope.rs Outdated Show resolved Hide resolved
sentry-types/src/protocol/envelope.rs Outdated Show resolved Hide resolved
@kamilogorek kamilogorek force-pushed the raw-envelope branch 2 times, most recently from a598c37 to 794c3f6 Compare June 17, 2022 12:11
@kamilogorek kamilogorek merged commit 992ae58 into master Jun 17, 2022
@kamilogorek kamilogorek deleted the raw-envelope branch June 17, 2022 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support sending a serialized envelope via Transport
3 participants