Include the length of time we have spent downloading DataStreams in the error message raised when we can't get the entire DataStream#707
Conversation
rhysparry
left a comment
rhysparry
left a comment
There was a problem hiding this comment.
✅ Approving. I think there is an opportunity to make this clearer by separating log context information from the data required to actually perform the work.
I'll leave it to you whether you want to do that in a follow up PR, this PR or if you think it's completely unnecessary.
| } | ||
|
|
||
| async Task ReadStreamAsync(string messageId, IReadOnlyList<DataStream> deserializedStreams, CancellationToken cancellationToken) | ||
| async Task ReadStreamAsync(string messageId, IReadOnlyList<DataStream> deserializedStreams, Stopwatch stopWatchForDownloadingAllStreams, CancellationToken cancellationToken) |
There was a problem hiding this comment.
We're passing around a lot of extra context information for the purposes of logging/enriching the exception message.
- Message Id
- Stopwatch
- Total size of data streams
It might help make this clearer if we wrapped all these in a small class. This would clearly separate data that is for logging and what is actually used in the logic. I would imagine it could also provide a ToString() implementation to simplify the addition of this context to all the various places.
2 participants
Background
ref CLOUDPT-11160
ref EFT-3141
In the issue we are looking at, it would be ideal to understand how long the service has been receiving DataStreams for. Since the client has given up waiting for the service to download the data streams.
Results
After
---> Halibut.Transport.Protocol.ProtocolException: Data stream reading failed. Message Id: IReadDataStreamService::SendDataAsync[1] / 0441e6e9-8aa4-4b57-b1ce-0925340cb159, Stream ID: e517a67d-e8ad-4af7-be18-f9748110fbc0, Expected length: 100, Actual bytes read: 18. Total length of all DataStreams to be sent is 100. Stream with length 100 was closed after only reading 18 bytes. Time elapsed downloading all streams: 44999ms.
How to review this PR
Quality ✔️
Pre-requisites