feat: updating proxy otel tracing#178
feat: updating proxy otel tracing#178brandtnewton wants to merge 5 commits intoGoogleCloudPlatform:mainfrom
Conversation
![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 integrates OpenTelemetry tracing into the Bigtable adapter and proxy handlers, adds support for the Google Cloud Trace exporter, and implements a parser pool to optimize performance. It also includes dependency updates, pprof profiling, and an increase in default gRPC channels. Review feedback highlights the need for more accurate span naming in generic execution paths, potential resource leaks in error handling where pooled parsers are not released, and the correction of unsafe fmt.Errorf calls. Using the provided Release abstraction on RawQuery is also suggested for better code clarity.
|
|
||
| var ( | ||
| spanExecutePreparedStatement = "executePreparedStatement" | ||
| spanMutateRow = "MutateRow" |
There was a problem hiding this comment.
The span name spanMutateRow ("MutateRow") is misleading when used in the generic Execute method, as it also handles SELECT, DELETE, and DDL operations. Renaming this constant to spanExecute with the value "Execute" improves code readability and accurately reflects the method's purpose.
| spanMutateRow = "MutateRow" | |
| spanExecute = "Execute" |
References
- Prioritize code readability over minor performance optimizations. Clear and accurate naming is essential for maintainable code.
| } | ||
|
|
||
| func (btc *BigtableAdapter) Execute(ctx context.Context, query types.IExecutableQuery) (message.Message, error) { | ||
| ctx, span := btc.otelInst.StartSpan(ctx, spanMutateRow, []attribute.KeyValue{ |
There was a problem hiding this comment.
Use a generic span name for the Execute method since it serves as the entry point for all query types. This improves the readability and logical flow of the tracing implementation.
| ctx, span := btc.otelInst.StartSpan(ctx, spanMutateRow, []attribute.KeyValue{ | |
| ctx, span := btc.otelInst.StartSpan(ctx, spanExecute, []attribute.KeyValue{ |
References
- Prioritize code readability over minor performance optimizations.
| res = BulkOperationResponse{ | ||
| FailedRows: fmt.Sprintf("failed rowkeys: %v", failedRows), | ||
| } | ||
| btc.otelInst.RecordError(span, fmt.Errorf(res.FailedRows)) |
There was a problem hiding this comment.
Using fmt.Errorf(res.FailedRows) is unsafe because res.FailedRows is a dynamically generated string that may contain format specifiers (e.g., from row keys). This can lead to unexpected formatting errors or panics. Use fmt.Errorf("%s", res.FailedRows) instead.
| btc.otelInst.RecordError(span, fmt.Errorf(res.FailedRows)) | |
| btc.otelInst.RecordError(span, fmt.Errorf("%s", res.FailedRows)) |
| if err != nil { | ||
| otelErr = err | ||
| c.proxy.otelInst.RecordError(span, otelErr) |
There was a problem hiding this comment.
The parser p is not released back to the pool if parseQueryType(p) fails. This results in a resource leak in the parser pool. Ensure the parser is released in the error path.
| if err != nil { | |
| otelErr = err | |
| c.proxy.otelInst.RecordError(span, otelErr) | |
| if err != nil { | |
| p.Release() | |
| otelErr = err | |
| c.proxy.otelInst.RecordError(span, otelErr) |
| if err != nil { | ||
| otelErr = err | ||
| c.proxy.otelInst.RecordError(span, otelErr) |
There was a problem hiding this comment.
The parser p is not released back to the pool if parseQueryType(p) fails, leading to a resource leak. Ensure the parser is released in the error path.
| if err != nil { | |
| otelErr = err | |
| c.proxy.otelInst.RecordError(span, otelErr) | |
| if err != nil { | |
| p.Release() | |
| otelErr = err | |
| c.proxy.otelInst.RecordError(span, otelErr) |
| } | ||
|
|
||
| func (t *TranslatorManager) TranslateQuery(q *types.RawQuery, sessionKeyspace types.Keyspace) (types.IPreparedQuery, error) { | ||
| defer q.Parser().Release() |
There was a problem hiding this comment.
Use q.Release() instead of q.Parser().Release(). The Release method on RawQuery is designed to return the parser to the pool and safely set the internal reference to nil, which is more readable and prevents potential use-after-release bugs.
| defer q.Parser().Release() | |
| defer q.Release() |
References
- Prioritize code readability over minor performance optimizations. Using the intended API abstraction improves clarity and safety.
1 participant
No description provided.