feat(storage): add grpc metrics experimental options#10984
Conversation
tritone
left a comment
There was a problem hiding this comment.
Looks great overall, thanks for figuring this out Frank!
| "google.golang.org/api/option" | ||
| ) | ||
|
|
||
| // WithMetricInterval how often to emit metrics when using NewPeriodicReader |
There was a problem hiding this comment.
Make it clear that these options are supposed to be used with storage.NewClient.
|
Also, looks like your unit test fails... can you investigate? |
tritone
left a comment
There was a problem hiding this comment.
Couple minor nits/questions, overall looks good
|
|
||
| // WithMetricInterval provides a [ClientOption] that may be passed to [storage.NewGrpcClient]. | ||
| // It sets how often to emit metrics when using NewPeriodicReader and must be | ||
| // greater than 1 minute. |
There was a problem hiding this comment.
Why is this the minimum? Not sure why this requires validation, users might have their own systems with higher rate limits.
There was a problem hiding this comment.
Good point; could pass along a value if it's non-zero.
| "google.golang.org/api/option" | ||
| ) | ||
|
|
||
| // WithMetricInterval provides a [ClientOption] that may be passed to [storage.NewGrpcClient]. |
There was a problem hiding this comment.
typo: should be storage.NewGRPCClient (here and below)
| // WithMetricInterval provides a [ClientOption] that may be passed to [storage.NewGrpcClient]. | ||
| // It sets how often to emit metrics when using NewPeriodicReader and must be | ||
| // greater than 1 minute. | ||
| // https://pkg.go.dev/go.opentelemetry.io/otel/sdk/metric#NewPeriodicReader |
There was a problem hiding this comment.
leave an empty line (with a // only) between links if you want them to not wrap in the godoc.
|
|
||
| // WithMetricExporter provides a [ClientOption] that may be passed to [storage.NewGrpcClient]. | ||
| // Set an alternate client-side metric Exporter to emit metrics through. | ||
| // Must implement interface metric.Exporter: |
There was a problem hiding this comment.
Just do [metric.Exporter] and the godoc should autolink.
| package storage | ||
|
|
||
| import ( | ||
| "time" |
There was a problem hiding this comment.
Not sure why this got moved but it will probably break formatting presubmits
| "google.golang.org/api/option" | ||
| ) | ||
|
|
||
| // WithMetricInterval provides a [ClientOption] that may be passed to [storage.NewGrpcClient]. |
There was a problem hiding this comment.
nit: what does ClientOption link to?
tritone
left a comment
There was a problem hiding this comment.
One more doc comment, otherwise LGTM
| // WithReadStallTimeout provides a [ClientOption] that may be passed to [storage.NewClient]. | ||
| // WithMetricInterval provides a [option.ClientOption] that may be passed to [storage.NewGRPCClient]. | ||
| // It sets how often to emit metrics [metric.WithInterval] when using | ||
| // [metric.NewPeriodicReader] |
There was a problem hiding this comment.
I think removing the minimum setting is good. Maybe suggest time.Minute or higher as an interval in the godoc if exporting to cloud monitoring.
No description provided.