feat(storage): add grpc metrics experimental options #10984
Conversation
product-auto-label
bot
added
the
api: storage
frankyn
changed the title
storage/experimental/experimental.go
Outdated
| "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
changed the title
storage/experimental/experimental.go
Outdated
| "google.golang.org/api/option" | ||
| ) | ||
|
|
||
| // 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.
storage/experimental/experimental.go
Outdated
| "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)
storage/experimental/experimental.go
Outdated
| // 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.
storage/experimental/experimental.go
Outdated
|
|
||
| // 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.
storage/option.go
Outdated
| @@ -15,12 +15,14 @@ | |||
| package storage | |||
|
|
|||
| import ( | |||
| "time" | |||
There was a problem hiding this comment.
Not sure why this got moved but it will probably break formatting presubmits
storage/experimental/experimental.go
Outdated
| "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?
| // 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.