feat: refine metrics #613
feat: refine metrics #613
Conversation
WalkthroughThe changes introduced in this pull request enhance the telemetry capabilities of the application by integrating OpenTelemetry for tracing and metrics. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ControllerWithTraces
participant MetricsProvider
User->>ControllerWithTraces: Call Method (e.g., BeginTX)
ControllerWithTraces->>MetricsProvider: Start Histogram Tracking
MetricsProvider-->>ControllerWithTraces: Return Histogram
ControllerWithTraces->>ControllerWithTraces: Perform Operation
ControllerWithTraces->>MetricsProvider: Stop Histogram Tracking
MetricsProvider-->>ControllerWithTraces: Log Metrics
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
internal/controller/ledger/controller_with_traces.go (2)
50-53: Avoid panicking on histogram initialization errorsUsing
panicfor error handling may cause the entire application to crash. Consider returning an error to handle histogram initialization failures gracefully.Modify the constructor to return an error:
-func NewControllerWithTraces(underlying Controller, tracer trace.Tracer, meter metric.Meter) *ControllerWithTraces { +func NewControllerWithTraces(underlying Controller, tracer trace.Tracer, meter metric.Meter) (*ControllerWithTraces, error) { ret := &ControllerWithTraces{ underlying: underlying, tracer: tracer, } var err error ret.beginTxHistogram, err = meter.Int64Histogram("BeginTX") if err != nil { - panic(err) + return nil, fmt.Errorf("failed to create histogram 'BeginTX': %w", err) } // ... repeat for other histograms - return ret + return ret, nil }Update the calls to
NewControllerWithTracesto handle the possible error.
49-138: Refactor histogram initialization to reduce duplicationThe repetitive initialization of histograms can be refactored to improve maintainability and reduce potential errors.
Example refactor:
operationNames := []struct { name string histogram *metric.Int64Histogram }{ {"BeginTX", &ret.beginTxHistogram}, {"Commit", &ret.commitHistogram}, {"Rollback", &ret.rollbackHistogram}, // ... add other operations } for _, op := range operationNames { *op.histogram, err = meter.Int64Histogram(op.name) if err != nil { return nil, fmt.Errorf("failed to create histogram '%s': %w", op.name, err) } }This approach reduces code duplication and simplifies the addition of new histograms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
internal/controller/ledger/controller_with_traces.go(2 hunks)internal/controller/system/controller.go(7 hunks)internal/controller/system/module.go(1 hunks)
🔇 Additional comments (11)
internal/controller/system/controller.go (10)
6-6: Importing OpenTelemetry attribute package
The addition of "go.opentelemetry.io/otel/attribute" is necessary for using OpenTelemetry attributes and enhances telemetry capabilities.
42-43: Replacing tracer and meter with provider patterns
Updating the DefaultController struct to use tracerProvider and meterProvider aligns with OpenTelemetry best practices and promotes consistent management of tracing and metrics.
48-48: Initializing tracer with provider in GetLedgerController
The use of ctrl.tracerProvider.Tracer("system") correctly initializes a tracer for the "system" instrumentation scope.
54-64: Initializing meter and tracer with instrumentation attributes
The code properly initializes meter and tracer with instrumentation attributes using the ledger name, enhancing observability and aligning with OpenTelemetry standards.
69-69: Passing meter to NewDefaultController
By passing the meter to ledgercontroller.NewDefaultController, the code enables metric collection within the ledger controller.
75-75: Passing tracer to NewControllerWithTooManyClientHandling
The tracer is correctly passed to enhance tracing capabilities in client handling.
89-89: Enhancing controller with traces and metrics
Wrapping the ledgerController with NewControllerWithTraces and providing both tracer and meter integrates tracing and metrics seamlessly.
170-172: Adding WithMeterProvider option
The WithMeterProvider function correctly allows setting the meterProvider in the DefaultController, following the options pattern and enhancing configurability.
176-178: Adding WithTracerProvider option
The WithTracerProvider function correctly sets the tracerProvider in the DefaultController, promoting consistent tracing configuration.
189-190: Setting default no-op providers
Assigning noopmetrics.MeterProvider{} and nooptracer.TracerProvider{} as defaults ensures the controller functions correctly even when no providers are explicitly supplied.
internal/controller/system/module.go (1)
51-52: Renaming parameters to WithMeterProvider and WithTracerProvider
Updating the parameter names enhances clarity by indicating that provider instances are being supplied, promoting consistency across the codebase.
| return tracing.SkipResult(tracing.TraceWithMetric( | ||
| ctx, | ||
| "BeginTX", | ||
| c.tracer, | ||
| c.commitHistogram, | ||
| tracing.NoResult(func(ctx context.Context) error { | ||
| return c.underlying.Commit(ctx) | ||
| }), | ||
| )) |
There was a problem hiding this comment.
Incorrect operation name in Commit method tracing
In the Commit method, the operation name passed to tracing.TraceWithMetric is "BeginTX". It should be "Commit" to reflect the actual operation.
Apply this diff to correct the operation name:
return tracing.SkipResult(tracing.TraceWithMetric(
ctx,
- "BeginTX",
+ "Commit",
c.tracer,
c.commitHistogram,
tracing.NoResult(func(ctx context.Context) error {
return c.underlying.Commit(ctx)
}),
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return tracing.SkipResult(tracing.TraceWithMetric( | |
| ctx, | |
| "BeginTX", | |
| c.tracer, | |
| c.commitHistogram, | |
| tracing.NoResult(func(ctx context.Context) error { | |
| return c.underlying.Commit(ctx) | |
| }), | |
| )) | |
| return tracing.SkipResult(tracing.TraceWithMetric( | |
| ctx, | |
| "Commit", | |
| c.tracer, | |
| c.commitHistogram, | |
| tracing.NoResult(func(ctx context.Context) error { | |
| return c.underlying.Commit(ctx) | |
| }), | |
| )) |
| ctx, | ||
| "CreateTransaction", | ||
| c.tracer, | ||
| c.revertTransactionHistogram, | ||
| func(ctx context.Context) (any, error) { | ||
| log, revertedTransaction, err = c.underlying.RevertTransaction(ctx, parameters) | ||
| return nil, err | ||
| }, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
Incorrect operation name in RevertTransaction method tracing
In the RevertTransaction method, the operation name is set to "CreateTransaction". It should be "RevertTransaction" to match the method's purpose.
Apply this diff to correct the operation name:
_, err = tracing.TraceWithMetric(
ctx,
- "CreateTransaction",
+ "RevertTransaction",
c.tracer,
c.revertTransactionHistogram,
func(ctx context.Context) (any, error) {
log, revertedTransaction, err = c.underlying.RevertTransaction(ctx, parameters)
return nil, err
},
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx, | |
| "CreateTransaction", | |
| c.tracer, | |
| c.revertTransactionHistogram, | |
| func(ctx context.Context) (any, error) { | |
| log, revertedTransaction, err = c.underlying.RevertTransaction(ctx, parameters) | |
| return nil, err | |
| }, | |
| ) | |
| if err != nil { | |
| ctx, | |
| "RevertTransaction", | |
| c.tracer, | |
| c.revertTransactionHistogram, | |
| func(ctx context.Context) (any, error) { | |
| log, revertedTransaction, err = c.underlying.RevertTransaction(ctx, parameters) | |
| return nil, err | |
| }, | |
| ) | |
| if err != nil { |
| commitHistogram metric.Int64Histogram | ||
| rollbackHistogram metric.Int64Histogram | ||
| listTransactionsHistogram metric.Int64Histogram | ||
| countTransactions metric.Int64Histogram |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent naming of countTransactions histogram field
The field countTransactions should be renamed to countTransactionsHistogram to maintain consistency with other histogram fields.
Apply this diff to rename the field:
-countTransactions metric.Int64Histogram
+countTransactionsHistogram metric.Int64HistogramAlso, update all references to this field in the code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| countTransactions metric.Int64Histogram | |
| countTransactionsHistogram metric.Int64Histogram |
| return tracing.SkipResult(tracing.TraceWithMetric( | ||
| ctx, | ||
| "BeginTX", | ||
| c.tracer, | ||
| c.rollbackHistogram, | ||
| tracing.NoResult(func(ctx context.Context) error { | ||
| return c.underlying.Rollback(ctx) | ||
| }), | ||
| )) |
There was a problem hiding this comment.
Incorrect operation name in Rollback method tracing
In the Rollback method, the operation name is mistakenly set to "BeginTX". It should be "Rollback" to accurately represent the operation.
Apply this diff to correct the operation name:
return tracing.SkipResult(tracing.TraceWithMetric(
ctx,
- "BeginTX",
+ "Rollback",
c.tracer,
c.rollbackHistogram,
tracing.NoResult(func(ctx context.Context) error {
return c.underlying.Rollback(ctx)
}),
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return tracing.SkipResult(tracing.TraceWithMetric( | |
| ctx, | |
| "BeginTX", | |
| c.tracer, | |
| c.rollbackHistogram, | |
| tracing.NoResult(func(ctx context.Context) error { | |
| return c.underlying.Rollback(ctx) | |
| }), | |
| )) | |
| return tracing.SkipResult(tracing.TraceWithMetric( | |
| ctx, | |
| "Rollback", | |
| c.tracer, | |
| c.rollbackHistogram, | |
| tracing.NoResult(func(ctx context.Context) error { | |
| return c.underlying.Rollback(ctx) | |
| }), | |
| )) |
gfyrag
force-pushed
the
feat/refine-metrics
branch
from
1da836b to
39006e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
internal/controller/ledger/controller_with_traces.go (1)
343-362: Standardize error handling patternThe error handling pattern varies across methods. Some use intermediate variables while others return directly.
Consider standardizing to the simpler direct return pattern:
-var ( - createdTransaction *ledger.CreatedTransaction - log *ledger.Log - err error -) -_, err = tracing.TraceWithMetric( +return tracing.TraceWithMetric( ctx, "CreateTransaction", c.tracer, c.createTransactionHistogram, - func(ctx context.Context) (any, error) { - log, createdTransaction, err = c.underlying.CreateTransaction(ctx, parameters) - return nil, err + func(ctx context.Context) (*ledger.Log, *ledger.CreatedTransaction, error) { + return c.underlying.CreateTransaction(ctx, parameters) }, ) -if err != nil { - return nil, nil, err -} -return log, createdTransaction, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
internal/controller/ledger/controller_with_traces.go(2 hunks)internal/controller/system/controller.go(7 hunks)internal/controller/system/module.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/system/module.go
🔇 Additional comments (5)
internal/controller/system/controller.go (3)
42-43: LGTM: Good transition to provider pattern
The change from direct tracer/meter to provider pattern aligns with OpenTelemetry best practices and enables better control over telemetry lifecycle.
54-64: LGTM: Well-structured telemetry setup
The implementation correctly creates scoped tracers and meters with proper instrumentation attributes, enabling per-ledger telemetry.
Line range hint 170-190: LGTM: Consistent provider pattern implementation
The option functions and defaults are correctly updated to match the provider pattern changes while maintaining backward compatibility.
internal/controller/ledger/controller_with_traces.go (2)
19-40: LGTM: Comprehensive telemetry coverage
The histogram fields provide good coverage for all operations, with consistent naming conventions.
343-362: LGTM: Operation names correctly aligned with methods
The operation names in CreateTransaction and RevertTransaction methods now correctly match their respective operations, addressing previous review comments.
Also applies to: 366-385
| return tracing.TraceWithMetric( | ||
| ctx, | ||
| "GetMigrationsInfo", | ||
| c.tracer, | ||
| c.listTransactionsHistogram, | ||
| func(ctx context.Context) ([]migrations.Info, error) { | ||
| return c.underlying.GetMigrationsInfo(ctx) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Fix incorrect histogram usage
The method is using listTransactionsHistogram for migration info operations, which is incorrect.
Add a dedicated histogram for migrations:
return tracing.TraceWithMetric(
ctx,
"GetMigrationsInfo",
c.tracer,
- c.listTransactionsHistogram,
+ c.getMigrationsInfoHistogram,
func(ctx context.Context) ([]migrations.Info, error) {
return c.underlying.GetMigrationsInfo(ctx)
},
)Committable suggestion skipped: line range outside the PR's diff.
| func NewControllerWithTraces(underlying Controller, tracer trace.Tracer, meter metric.Meter) *ControllerWithTraces { | ||
| ret := &ControllerWithTraces{ | ||
| underlying: underlying, | ||
| tracer: tracer, | ||
| } | ||
|
|
||
| var err error | ||
| ret.beginTxHistogram, err = meter.Int64Histogram("BeginTX") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.listTransactionsHistogram, err = meter.Int64Histogram("ListTransactions") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.commitHistogram, err = meter.Int64Histogram("Commit") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.rollbackHistogram, err = meter.Int64Histogram("Rollback") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.countTransactionsHistogram, err = meter.Int64Histogram("CountTransactions") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.getTransactionHistogram, err = meter.Int64Histogram("GetTransaction") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.countAccountsHistogram, err = meter.Int64Histogram("CountAccounts") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.listAccountsHistogram, err = meter.Int64Histogram("ListAccounts") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.getAccountHistogram, err = meter.Int64Histogram("GetAccount") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.getAggregatedBalancesHistogram, err = meter.Int64Histogram("GetAggregatedBalances") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.listLogsHistogram, err = meter.Int64Histogram("ListLogs") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.importHistogram, err = meter.Int64Histogram("Import") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.exportHistogram, err = meter.Int64Histogram("Export") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.isDatabaseUpToDateHistogram, err = meter.Int64Histogram("IsDatabaseUpToDate") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.getVolumesWithBalancesHistogram, err = meter.Int64Histogram("GetVolumesWithBalances") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.getStatsHistogram, err = meter.Int64Histogram("GetStats") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.createTransactionHistogram, err = meter.Int64Histogram("CreateTransaction") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.revertTransactionHistogram, err = meter.Int64Histogram("RevertTransaction") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.saveTransactionMetadataHistogram, err = meter.Int64Histogram("SaveTransactionMetadata") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.saveAccountMetadataHistogram, err = meter.Int64Histogram("SaveAccountMetadata") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.deleteTransactionMetadataHistogram, err = meter.Int64Histogram("DeleteTransactionMetadata") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| ret.deleteAccountMetadataHistogram, err = meter.Int64Histogram("DeleteAccountMetadata") | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor constructor to improve error handling and reduce duplication
The constructor has several issues:
- Using panic for error handling
- Repetitive error handling code
- Long and hard to maintain
Consider this refactoring:
func NewControllerWithTraces(underlying Controller, tracer trace.Tracer, meter metric.Meter) *ControllerWithTraces {
+ createHistogram := func(name string) metric.Int64Histogram {
+ h, err := meter.Int64Histogram(name)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create histogram %s: %w", name, err)
+ }
+ return h
+ }
ret := &ControllerWithTraces{
underlying: underlying,
tracer: tracer,
}
- var err error
- ret.beginTxHistogram, err = meter.Int64Histogram("BeginTX")
- if err != nil {
- panic(err)
- }
+ histograms := map[string]*metric.Int64Histogram{
+ "BeginTX": &ret.beginTxHistogram,
+ "ListTransactions": &ret.listTransactionsHistogram,
+ // ... other histograms
+ }
+
+ for name, hist := range histograms {
+ *hist, err = createHistogram(name)
+ if err != nil {
+ return nil, err
+ }
+ }
return ret
}Committable suggestion skipped: line range outside the PR's diff.
No description provided.