INFOPLAT-2288: Add grpc metrics to Beholder using StatsHandler #1617
Conversation
|
👋 kirqz23, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
✅ API Diff Results - No breaking changes |
pkg/beholder/client.go
Outdated
| otlptracegrpc.WithTLSCredentials(creds), | ||
| otlptracegrpc.WithEndpoint(config.OtelExporterGRPCEndpoint), | ||
| // Uses deferred binding for global providers | ||
| otlptracegrpc.WithDialOption(grpc.WithStatsHandler(otelgrpc.NewClientHandler())), |
There was a problem hiding this comment.
it should not be applied to meter and tracer providers because otelgrpc uses them actually under the hood
pkg/beholder/client.go
Outdated
| otlpmetricgrpc.WithTLSCredentials(creds), | ||
| otlpmetricgrpc.WithEndpoint(cfg.OtelExporterGRPCEndpoint), | ||
| // Uses deferred binding for global providers | ||
| otlpmetricgrpc.WithDialOption(grpc.WithStatsHandler(otelgrpc.NewClientHandler())), |
There was a problem hiding this comment.
pkg/beholder/client.go
Outdated
| } | ||
| } | ||
|
|
||
| opts := []otlploggrpc.Option{ |
There was a problem hiding this comment.
#nit: should be renamed to logExporterOpts or something more explicit
Its used here https://github.com/smartcontractkit/chainlink-common/pull/1617/files#diff-74ce7f3d5474f67956a5d9ba5779b478b715235a0d33e8826eb99b146664ae4dR146 for sharedLogExporter
pkg/beholder/client.go
Outdated
| otlploggrpc.WithTLSCredentials(creds), | ||
| otlploggrpc.WithEndpoint(cfg.OtelExporterGRPCEndpoint), | ||
| // Uses deferred binding for global providers | ||
| otlploggrpc.WithDialOption(grpc.WithStatsHandler(otelgrpc.NewClientHandler())), |
There was a problem hiding this comment.
You need to pass tracer and meeter providers here to NewClientHandler
See example https://github.com/smartcontractkit/chainlink-common/pull/1613/files#diff-6412d8ee68e9ba61be413daa002ec8924fd9ca953fef0376e0a0c8de68509e5fR78-R83
There was a problem hiding this comment.
That would require changing order how things are initialized in NewGRPCClient
It should be metrics, tracing first than logger.
There was a problem hiding this comment.
#nit: Maybe it worth creating a helper function newLoggerProvider for logger similar to how its done for metrics and traces
…emitter using shared exporter with grpc metrics
kirqz23
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 28f28bd | Previous: 2c2872d | Ratio |
|---|---|---|---|
BenchmarkKeystore_Sign/nop/in-process |
786.2 ns/op |
373.4 ns/op |
2.11 |
This comment was automatically generated by workflow using github-action-benchmark.
| messageAttributes := []attribute.KeyValue{ | ||
| attribute.String(AttrKeyDataType, "custom_message"), | ||
| } | ||
| messageLoggerResource, err := sdkresource.Merge( | ||
| sdkresource.NewSchemaless(messageAttributes...), |
There was a problem hiding this comment.
Is this a valid simplification?
| messageAttributes := []attribute.KeyValue{ | |
| attribute.String(AttrKeyDataType, "custom_message"), | |
| } | |
| messageLoggerResource, err := sdkresource.Merge( | |
| sdkresource.NewSchemaless(messageAttributes...), | |
| messageLoggerResource, err := sdkresource.Merge( | |
| sdkresource.NewSchemaless(attribute.String(AttrKeyDataType, "custom_message")), |
| loggerAttributes := []attribute.KeyValue{ | ||
| attribute.String(AttrKeyDataType, "zap_log_message"), | ||
| } | ||
| loggerResource, err := sdkresource.Merge( | ||
| sdkresource.NewSchemaless(loggerAttributes...), |
There was a problem hiding this comment.
| loggerAttributes := []attribute.KeyValue{ | |
| attribute.String(AttrKeyDataType, "zap_log_message"), | |
| } | |
| loggerResource, err := sdkresource.Merge( | |
| sdkresource.NewSchemaless(loggerAttributes...), | |
| loggerResource, err := sdkresource.Merge( | |
| sdkresource.NewSchemaless(attribute.String(AttrKeyDataType, "zap_log_message")), |
| otelOpts := []otelgrpc.Option{ | ||
| otelgrpc.WithMeterProvider(meter), | ||
| otelgrpc.WithTracerProvider(tracer), | ||
| } | ||
|
|
||
| opts := []otlploggrpc.Option{ | ||
| otlploggrpc.WithTLSCredentials(creds), | ||
| otlploggrpc.WithEndpoint(cfg.OtelExporterGRPCEndpoint), | ||
| otlploggrpc.WithDialOption(grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelOpts...))), |
There was a problem hiding this comment.
nit/ could simplify and reduce nesting:
| otelOpts := []otelgrpc.Option{ | |
| otelgrpc.WithMeterProvider(meter), | |
| otelgrpc.WithTracerProvider(tracer), | |
| } | |
| opts := []otlploggrpc.Option{ | |
| otlploggrpc.WithTLSCredentials(creds), | |
| otlploggrpc.WithEndpoint(cfg.OtelExporterGRPCEndpoint), | |
| otlploggrpc.WithDialOption(grpc.WithStatsHandler(otelgrpc.NewClientHandler(otelOpts...))), | |
| otelClient := otelgrpc.NewClientHandler(otelgrpc.WithMeterProvider(meter), otelgrpc.WithTracerProvider(tracer)) | |
| opts := []otlploggrpc.Option{ | |
| otlploggrpc.WithTLSCredentials(creds), | |
| otlploggrpc.WithEndpoint(cfg.OtelExporterGRPCEndpoint), | |
| otlploggrpc.WithDialOption(grpc.WithStatsHandler(otelClient)), |
3 participants
This PR splits some code into separate functions:
newLoggerExporter,newLoggerProviderandnewMessageLoggerProvider. Additionally it adds gRPC instrumentation to the Beholder client to enable automatic collection of gRPC performance metrics usingStatsHandler. The instrumentation provides visibility into gRPC request/response sizes, durations, success rates.Related to: smartcontractkit/chainlink#19089