-
Notifications
You must be signed in to change notification settings - Fork 133
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
RUM-1000 chore: improve telemetry messages for file I/O errors #1922
Conversation
DD.logger.error("Failed to get writable file", error: error) | ||
telemetry.error("Failed to get writable file", error: error) |
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.
suggestion/ For more context, would it make sense to include writeSize
? To control the cardinality of error.message
, this could be done by composing the error
:
enum FileWriterError: Error { /* ... */ }
FileWriterError.writeError(error: error, writeSize: writeSize)
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.
this would mask the underlying error and wrapping one error inside another is something not very encouraged. However I do like the idea of using the writeSize in the error message. Given these are logs, cardinality must not be an issue.
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.
I like the direction - definitely better than what we have today. However, to me the most critical piece that is missing is the actual track we write to (rum
, trace
, logs
, sr
, sr-resources
). Given that we have trackName
available in parent FilesOrchestrator
, could we make it into FileWriter
and attach to the error
?
fair call, I just updated error messages with track name following the same format as we do at other places |
cd5a2f9
to
b1ce789
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.
Great! 🎯
What and why?
Current telemetry message is not very helpful to diagnose file IO related issues
For example
How?
Split the big
do-try-catch
block into individual ones to get more insights where the issue.Review checklist
Custom CI job configuration (optional)
tools/