-
-
Notifications
You must be signed in to change notification settings - Fork 225
Don't resend envelopes that get rejected by the server #3938
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
Changes from 3 commits
31169c3
75ad4c8
2529ab5
53de776
ff79666
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -300,13 +300,22 @@ private async Task ProcessCacheAsync(CancellationToken cancellation) | |
| } | ||
| } | ||
|
|
||
| private static bool IsNetworkError(Exception exception) => | ||
| private static bool IsNetworkUnavailableError(Exception exception) => | ||
| exception switch | ||
| { | ||
| // TODO: Could we make this more specific? Lots of these errors are unrelated to network availability. | ||
| HttpRequestException or WebException or IOException or SocketException => true, | ||
| _ => false | ||
| }; | ||
|
|
||
| private static bool IsRejectedByServer(Exception ex) | ||
| { | ||
| // When envelopes are too big, the server will reset the connection as soon as the maximum size is exceeded | ||
| // (it doesn't wait for us to finish sending the whole envelope). | ||
| return ex is SocketException { ErrorCode: 32 /* Broken pipe */ } | ||
| || (ex.InnerException is { } innerException && IsRejectedByServer(ex.InnerException)); | ||
jamescrosswell marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| private async Task InnerProcessCacheAsync(string file, CancellationToken cancellation) | ||
| { | ||
| if (_options.NetworkStatusListener is { Online: false } listener) | ||
|
|
@@ -346,8 +355,15 @@ private async Task InnerProcessCacheAsync(string file, CancellationToken cancell | |
| // Let the worker catch, log, wait a bit and retry. | ||
| throw; | ||
| } | ||
| catch (Exception ex) when (IsNetworkError(ex)) | ||
| catch (Exception ex) when (IsRejectedByServer(ex)) | ||
| { | ||
| _options.ClientReportRecorder.RecordDiscardedEvents(DiscardReason.SendError, envelope); | ||
| _options.LogError(ex, "Failed to send cached envelope: {0}. The envelope is likely too big and will be discarded.", file); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we have a specific "too big" client report? It's possible we'll get that from the server though in which case this is fine. But when debugging data loss on behalf of customers, when we see "send error" it's often hard to know what to do next. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just heard that Relay doesn't report anything btw. So we must There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't no. According to the docs:
I couldn't see anything that matched up neatly with the situation we're experiencing so I used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there wasn't anything better, I went with |
||
| } | ||
| catch (Exception ex) when (IsNetworkUnavailableError(ex)) | ||
| { | ||
| // TODO: Envelopes could end up in an infinite loop here. We should consider implementing some | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could end up with a lot of items in queue causing new data to not get a chance to be pushed on mobile platforms. Can the queue be sorted to send new data first until a retry/circuit breaker can be added? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Priority queues (e.g. prioritise error reports over traces) is one for the todo list: |
||
| // kind backoff strategy and a retry limit... then drop the envelopes if the limit is exceeded. | ||
| if (_options.NetworkStatusListener is PollingNetworkStatusListener pollingListener) | ||
| { | ||
| pollingListener.Online = false; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.