Add interface for metric instantiation#654
Conversation
monitoring/metrics.go
Outdated
|
|
||
| // MetricFactory allows the creation of different types of metric. | ||
| type MetricFactory interface { | ||
| NewCounter(name, help string, labelNames []string) Counter |
There was a problem hiding this comment.
Why not use labelNames ...string here?
| // Counter is a wrapper around a Prometheus Counter or CounterVec object. | ||
| type Counter struct { | ||
| labelNames []string | ||
| single prometheus.Counter |
There was a problem hiding this comment.
Shouldn't this be by reference? Or it's not needed because prometheus.Counter is an interface (and they're always references)?
|
|
||
| func (m *memoryLogStorage) beginInternal(ctx context.Context, treeID int64, readonly bool) (storage.LogTreeTX, error) { | ||
| once.Do(func() { | ||
| createMetrics(prometheus.MetricFactory{}) |
There was a problem hiding this comment.
Shouldn't that come from the registry, rather than have prometheus hardcoded?
There was a problem hiding this comment.
Ideally, yes -- I'll add a TODO.
|
|
||
| func (m *mySQLLogStorage) beginInternal(ctx context.Context, treeID int64, readonly bool) (storage.LogTreeTX, error) { | ||
| once.Do(func() { | ||
| createMetrics(prometheus.MetricFactory{}) |
There was a problem hiding this comment.
Same question here, shouldn't that use the registry?
There was a problem hiding this comment.
Ideally, yes, although in practice I don't think we'll ever use the MySQL storage layer with anything other than the Prometheus metric layer.
I'll add a TODO
| return nil, fmt.Errorf("Unsequenced: %v", err) | ||
| } | ||
| } | ||
| queuedCounter.Add(float64(len(leaves)), labelForTX(t)) |
There was a problem hiding this comment.
I'll trust you know what you're doing with the changes here and below... 😃
monitoring/rpc_stats_interceptor.go
Outdated
| r.handlerRequestErrorCountMap.Add(method, 1) | ||
| r.handlerRequestFailedLatencyMap.Add(method, latency.Nanoseconds()/nanosToMillisDivisor) | ||
| r.ReqErrorCount.Inc(labels...) | ||
| r.ReqErrorLatency.Observe(float64(latency.Nanoseconds()/int64(time.Millisecond)), labels...) |
There was a problem hiding this comment.
Shouldn't latency.Nanoseconds() divide by time.Millisecond without the need for casts? And then cast back to float64 once? I admit there's the semantic ickiness of momentarily having a time.Duration that's "incorrect" (containing a value in the wrong unit)...
There was a problem hiding this comment.
latency.Nanoseconds() is an int64, but you're right, it can be simplified to float64(latency/time.Millisecond) - done.
New interface to create different sorts of metric, each of which is an interface. Add a testonly/ package with tests that should pass regardless of the implementation of the interface.
Add an in-memory, unexported implementation of the MetricFactory interfaces. This is only useful for tests.
Add the MetricFactory interface to the extension registry, and use the Prometheus-based implementation in the logsigner and logserver.
Needed to expose metrics.
Use the monitoring.MetricFactory interface instead. Move the RPC interceptor test into the monitoring_test package as it needs a concrete implementation (and only needs public entrypoints from the monitoring package anyway).
If so configured, announce HTTP endpoints for the logserver and the logsigner to etcd. This allows monitoring systems to discover those endpoints.
daviddrysdale
force-pushed
the
wrapped-metrics
branch
from
3d8cd44 to
82df669
Compare
- catch-up with google#654 (where the flag was removed) - needed for google/keytransparency#578
- catch-up with #654 (where the flag was removed) - needed for google/keytransparency#578
3 participants
No description provided.