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

RUM-1000 chore: improve telemetry messages for file I/O errors #1922

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

ganeshnj
Copy link
Contributor

What and why?

Current telemetry message is not very helpful to diagnose file IO related issues

For example

Failed to write data to file - Cannot create file at path: /var/mobile/Containers/Data/Application/*/Library/Caches

How?

Split the big do-try-catch block into individual ones to get more insights where the issue.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Session Replay
  • Run smoke tests
  • Run tests for tools/

@ganeshnj ganeshnj requested review from a team as code owners June 24, 2024 09:50
mariedm
mariedm previously approved these changes Jun 27, 2024
Comment on lines 68 to 69
DD.logger.error("Failed to get writable file", error: error)
telemetry.error("Failed to get writable file", error: error)
Copy link
Collaborator

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)

Copy link
Contributor Author

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.

Copy link
Collaborator

@ncreated ncreated left a 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?

@ganeshnj
Copy link
Contributor Author

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 ({trackname})

@ganeshnj ganeshnj force-pushed the ganeshnj/chore/RUM-100-improve-telemetry branch from cd5a2f9 to b1ce789 Compare June 28, 2024 13:04
Copy link
Collaborator

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Great! 🎯

@ganeshnj ganeshnj merged commit 7c8a559 into develop Jun 28, 2024
10 checks passed
@ganeshnj ganeshnj deleted the ganeshnj/chore/RUM-100-improve-telemetry branch June 28, 2024 13:39
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.

None yet

3 participants