Instrument write flows with OpenTelemetry traces#4506
Instrument write flows with OpenTelemetry traces#4506thrivikram-karur-g wants to merge 4 commits intomasterfrom
Conversation
Adds new traces to the write flow paths in GCSFuse, including both the legacy staged writes and the new streaming writes. This enables better observability into performance bottlenecks during write operations and file synchronization. A new documentation file, docs/write-flow-traces.md, was also added to guide users on how to use these new spans for debugging common performance scenarios related to GCSFuse write behavior. Co-authored-by: thrivikram-karur-g <235283545+thrivikram-karur-g@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
github-actions
Bot
added
the
remind-reviewers
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances GCSFuse's observability by integrating OpenTelemetry tracing into its file write mechanisms. This provides critical insights into the performance characteristics of both staged and streaming write operations, enabling more effective diagnosis of latency issues and operational anomalies. The accompanying documentation serves as a guide for leveraging these new traces to troubleshoot common scenarios, improving the overall debuggability of write flows. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces documentation for OpenTelemetry traces in GCSFuse's write flows. The new documentation is clear, well-structured, and provides valuable guidance for debugging performance issues related to file writes. I've provided a couple of suggestions to improve the consistency and accuracy of the trace descriptions and their usage in the debugging scenarios.
| | `fs.file.write.staged` | `internal/fs/inode/file.go` | Traces a write operation using the legacy staged writes (`writeUsingTempFile`). | Understanding the latency of writing to the local temporary file buffer, distinguishing it from streaming writes latency. | | ||
| | `fs.file.write.streaming` | `internal/fs/inode/file.go` | Traces a write operation using the buffered writes handler (`writeUsingBufferedWrites`). | Tracking the time taken to buffer the write locally before uploading. Helps identify if the buffer pool is exhausted and the write is blocking. | | ||
| | `fs.file.sync.staged` | `internal/fs/inode/file.go` | Traces the synchronization of the staged temp file to GCS (`syncUsingContent`). | Measuring the total time taken to upload the staged file to GCS. Helps distinguish local write time from upload time, identifying slow GCS connections. | | ||
| | `fs.file.sync.streaming` | `internal/fs/inode/file.go` | Traces the synchronization of buffered writes to GCS (`SyncPendingBufferedWrites`). | Measuring the time waiting for in-flight streaming blocks to finish uploading to GCS during a file sync or close. | |
There was a problem hiding this comment.
The description for fs.file.sync.streaming seems a bit too specific, mentioning only SyncPendingBufferedWrites. The fallback scenario on line 29 implies that flushUsingBufferedWriteHandler is also covered by this trace. To improve clarity, I suggest broadening the description to include other synchronization scenarios for streaming writes.
| | `fs.file.sync.streaming` | `internal/fs/inode/file.go` | Traces the synchronization of buffered writes to GCS (`SyncPendingBufferedWrites`). | Measuring the time waiting for in-flight streaming blocks to finish uploading to GCS during a file sync or close. | | |
| | `fs.file.sync.streaming` | `internal/fs/inode/file.go` | Traces the synchronization of buffered writes to GCS (e.g., `SyncPendingBufferedWrites`, `flushUsingBufferedWriteHandler`). | Measuring the time waiting for in-flight streaming blocks to finish uploading to GCS during a file sync or close. | |
|
|
||
| ### 2. High latency during file close/sync | ||
| * **Symptom:** Saving a file takes a long time, freezing the application briefly. | ||
| * **Trace Analysis:** Check `fs.file.sync.staged` and `fs.file.sync.streaming`. High latency in `SyncFileStaged` means the entire file is being uploaded to GCS at the time of closing. High latency in `SyncFileStreaming` indicates that the background upload process is slow, or `StreamingUploadFinalize` is blocking waiting for blocks. Check `streaming.block.upload` for individual block latencies. |
There was a problem hiding this comment.
For better clarity and consistency, it's best to use the exact trace names (wrapped in backticks) as defined in the table when discussing them in the analysis. This helps users quickly identify the correct spans in their tracing tool.
| * **Trace Analysis:** Check `fs.file.sync.staged` and `fs.file.sync.streaming`. High latency in `SyncFileStaged` means the entire file is being uploaded to GCS at the time of closing. High latency in `SyncFileStreaming` indicates that the background upload process is slow, or `StreamingUploadFinalize` is blocking waiting for blocks. Check `streaming.block.upload` for individual block latencies. | |
| * **Trace Analysis:** Check `fs.file.sync.staged` and `fs.file.sync.streaming`. High latency in `fs.file.sync.staged` means the entire file is being uploaded to GCS at the time of closing. High latency in `fs.file.sync.streaming` indicates that the background upload process is slow, or `streaming.upload.finalize` is blocking waiting for blocks. Check `streaming.block.upload` for individual block latencies. |
Adds new traces to the write flow paths in GCSFuse, including both the legacy staged writes and the new streaming writes. This enables better observability into performance bottlenecks during write operations and file synchronization. A new documentation file, docs/write-flow-traces.md, was also added to guide users on how to use these new spans for debugging common performance scenarios related to GCSFuse write behavior. Co-authored-by: thrivikram-karur-g <235283545+thrivikram-karur-g@users.noreply.github.com>
Adds new traces to the write flow paths in GCSFuse, including both the legacy staged writes and the new streaming writes. This enables better observability into performance bottlenecks during write operations and file synchronization. A new documentation file, docs/write-flow-traces.md, was also added to guide users on how to use these new spans for debugging common performance scenarios related to GCSFuse write behavior. Co-authored-by: thrivikram-karur-g <235283545+thrivikram-karur-g@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4506 +/- ##
==========================================
+ Coverage 83.67% 83.72% +0.04%
==========================================
Files 163 163
Lines 20089 20112 +23
==========================================
+ Hits 16810 16838 +28
+ Misses 2653 2650 -3
+ Partials 626 624 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds new traces to the write flow paths in GCSFuse, including both the legacy staged writes and the new streaming writes. This enables better observability into performance bottlenecks during write operations and file synchronization. A new documentation file, docs/write-flow-traces.md, was also added to guide users on how to use these new spans for debugging common performance scenarios related to GCSFuse write behavior. Co-authored-by: thrivikram-karur-g <235283545+thrivikram-karur-g@users.noreply.github.com>
|
Hi @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Closing this as raising a new cleaner pull request for the write traces |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
This submission adds OpenTelemetry tracing to the file write paths (staged and streaming) as well as generating documentation around it.
PR created automatically by Jules for task 17287576163767606080 started by @thrivikram-karur-g