feat: Drop invalid attachments #1134
Conversation
SentryEnvelopeItem.fromAttachment throws now a SentryEnvelopeException when it fails to create an envelope item from an attachment. The GsonSerializer logs the exception and drops the envelope item. So we avoid ingesting empty attachments. Fixes GH-1123
philipphofmann
requested review from
bruno-garcia,
maciejwalkowiak and
marandaneto
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1134 +/- ##
============================================
+ Coverage 74.93% 75.06% +0.12%
- Complexity 1606 1612 +6
============================================
Files 165 167 +2
Lines 5593 5602 +9
Branches 547 545 -2
============================================
+ Hits 4191 4205 +14
+ Misses 1150 1146 -4
+ Partials 252 251 -1
Continue to review full report at Codecov.
|
| gson.toJson(item.getHeader(), SentryEnvelopeItemHeader.class, writer); | ||
| writer.write("\n"); | ||
| writer.flush(); | ||
| try { |
There was a problem hiding this comment.
It's open up for the case of sending envelopes without items but if the stream in question is the network stream, it would be too late to do anything about it anyway unless we supported some trailing header to communicate the failure
There was a problem hiding this comment.
I think its already done but if not yet, it'd be nice to check if the envelope items are not empty before calling this method
There was a problem hiding this comment.
@marandaneto what exactly do you mean by checking if the envelope items are not empty?
There was a problem hiding this comment.
Good point @bruno-garcia, but I think we already had the problem before. I think this change slightly improves it.
Given an envelope with two items and the first to send throws an exception.
Old
Send header
Send 1st item -> Exception
Stop
We just sent an envelope without items.
New
Send header
Send 1st item -> Exception, Drop the item
Send 2nd
We send the envelope but dropped the 1st item.
Given an envelope with one item that throws.
Old and New
Send header
Send 1st item -> Exception
We just sent an envelope without items.
For output streams that are all or nothing we just changed the behavior, because if there is one error in any of the items we previously discarded the whole envelope and now we keep the envelope. Therefore we keep an envelope without any items. I think we could solve this by throwing an exception when we serialize an envelope and we can't serialize a single item, but according to our docs an envelope with just a header, such as {"event_id":"12c2d058d58442709aa2eca08bf20986"} is still a valid envelope: https://develop.sentry.dev/sdk/envelopes/#envelopes. Valid but useless.
There was a problem hiding this comment.
@marandaneto what exactly do you mean by checking if the envelope items are not empty?
I just checked up and we don't do it:
we could do:
if (!envelope.getItems().isEmpty()) {
executor.submit(new EnvelopeSender(envelope, hint, currentEnvelopeCache));
}
ps: getItems is an iterator so I don't think isEmpty would work, just a pseudocode
so we'd never try to send an envelope without items.
There was a problem hiding this comment.
I think we already check that in
, so I think it is not needed.There was a problem hiding this comment.
@philipphofmann this condition is only met if at least 1 item of the envelope is rate limited
There was a problem hiding this comment.
it's not related to this PR though, and the backend accepts an empty envelope, so feel free to ignore it, I just wrote it because I thought that the backend would trigger an error, it doesn't, as said by Armin.
| private lateinit var logger: ILogger | ||
| private lateinit var serializer: GsonSerializer | ||
|
|
||
| @BeforeTest | ||
| fun before() { | ||
| logger = mock() | ||
| serializer = GsonSerializer(logger, EnvelopeReader()) | ||
| } |
There was a problem hiding this comment.
M: what's about refactoring this and adding the Fixture?
📜 Description
SentryEnvelopeItem.fromAttachment throws now a SentryEnvelopeException when it fails
to create an envelope item from an attachment. The GsonSerializer logs the exception
and drops the envelope item. So we avoid ingesting empty attachments.
This should have been a draft PR. It is not completely finished yet.
💡 Motivation and Context
Fixes GH-1123
💚 How did you test it?
Unit tests and emulator
📝 Checklist
🔮 Next steps