-
Notifications
You must be signed in to change notification settings - Fork 472
feat(tracing): Add read flow traces #4244
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
base: master
Are you sure you want to change the base?
Conversation
…CloudPlatform/gcsfuse into vikram-add-read-traces
Summary of ChangesHello @thrivikram-karur-g, 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 the observability of read operations by introducing a new tracing framework based on OpenTelemetry. It integrates tracing into critical read paths, including file caching and direct GCS reads, to provide detailed insights into performance and potential bottlenecks. The changes allow for better understanding and debugging of how data is accessed and processed within the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces distributed tracing for read flows by adding a new tracing package and integrating it throughout the read path. The changes are well-structured, introducing a TraceHandle interface with noop and otel implementations, and using it to trace operations like file cache reads and downloads.
I have a few suggestions to improve the trace context propagation and the consistency of span naming. Overall, this is a great addition for observability.
| // the file, capped by the configured minimum. | ||
| blocksInFile := (int64(opts.Object.Size) + opts.Config.PrefetchBlockSizeBytes - 1) / opts.Config.PrefetchBlockSizeBytes | ||
| numBlocksToReserve := min(blocksInFile, opts.Config.MinBlocksPerHandle) | ||
| _, span := opts.TraceHandle.StartTrace(context.Background(), tracing.ReadPrefetchBlockPoolGen) |
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.
Using context.Background() here starts a new trace, which will not be a child of the parent operation's trace (e.g., ReadFile). This breaks the trace context propagation.
To fix this, the context.Context from the ReadWithReadManager call in internal/fs/handle/file.go should be propagated down to NewBufferedReader. This can be done by:
- Adding a
Ctx context.Contextfield toread_manager.ReadManagerConfigandbufferedread.BufferedReaderOptions. - Updating
read_manager.NewReadManagerto accept a context and pass it through the options. - Using the propagated context here instead of
context.Background().
This will ensure that the ReadPrefetchBlockPoolGen span is correctly parented under the ongoing operation's span.
|
|
||
| var err error | ||
| for _, r := range rr.readers { | ||
| ctx, span := rr.traceHandle.StartTrace(ctx, reflect.TypeOf(r).String()) |
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.
Using reflect.TypeOf(r).String() for span names makes the tracing dependent on the internal type names, which can be brittle. For example, if a type is renamed or moved, the span name will change unexpectedly. A more robust approach would be to use string constants for span names for each reader type.
Description
Adding read flow traces for flows like file cache read, file cache write, file cache with parallel downloads,
Link to the issue in case of a bug fix.
b/454481823
Testing details
Any backward incompatible change? If so, please explain.
NA