Unify client metrics with other metrics approach

[Bez625]

No description provided.

[mcdee]

Not sure about this one. This change hardcodes the prometheus metrics instance through the import

clientprometheus "github.com/attestantio/vouch/services/metrics/prometheus"

So we lose the flexibility of having other styles of metrics in the future. I'll have a bit more of a think about it, but I'm inclined to keep the client monitor interface and instantiate as we do at the moment.

[Bez625]

Not sure about this one. This change hardcodes the prometheus metrics instance through the import

clientprometheus "github.com/attestantio/vouch/services/metrics/prometheus"

So we lose the flexibility of having other styles of metrics in the future. I'll have a bit more of a think about it, but I'm inclined to keep the client monitor interface and instantiate as we do at the moment.

I understand the point about flexibility, but we have moved to the prometheus client being hardcoded for all of the other metrics following the pattern in https://github.com/attestantio/vouch/blob/master/services/beaconblockproposer/standard/metrics.go.

It feels inconsistent to have the two different ways of working with metrics - is there a reason we would want the flexibility on client/strategy metrics alone and have the others follow the hard coded approach?

i had a think and the only way I can see for us to get flexibility without having a massive interface, or a collection of small interfaces all implemented by one struct, is to have a small generic interface like:

type Metrics interface {
    Observe(ctx context.Context, metricName string, startTime float64)
    SetGauge(ctx context.Context, metricName string, value float64)
    Inc(ctx context.Context, metricName string, labels ...string)
}

And this could be used in a similar way to now:

// monitorBeaconBlockProposalCompleted is called when a block proposal process has completed.
func (s *Service) monitorBeaconBlockProposalCompleted(ctx context.Context, started time.Time, slot phase0.Slot, startOfSlot time.Time, result string) {
    // Only log times for successful completions.
    if result == "succeeded" {
        s.Metrics.Observe(ctx, "vouch_beaconblockproposal_process_duration_seconds", time.Since(started).Seconds())
        secsSinceStartOfSlot := time.Since(startOfSlot).Seconds()
        s.Metrics.Observe(ctx, "vouch_beaconblockproposal_mark_seconds", secsSinceStartOfSlot)
        s.Metrics.SetGauge(ctx, "vouch_beaconblockproposal_process_latest_slot", float64(slot))
    }
    s.Metrics.Inc(ctx, "vouch_beaconblockproposal_process_requests_total", result)
}

Obviously we would need to perform some kind of metrics lookup in the implementation of the interface, which could introduce a risk of runtime failures to find the metrics. There is also the risk that the interface may not have appropriate params for a future metrics client, but it might give us a way to keep flexibility without having large interfaces.