Pass in metrics server to avssync #34
Conversation
There was a problem hiding this comment.
Changes look fine to me, but why do you need this exactly?
If you want to run this as a binary and share metrics with another service/goroutine, then typically you would want to share the prometheus registry instead I feel? How else are you thinking of creating a metrics server outside, and why is it useful?
The use case we're thinking of is running the avssync package as part of another service/binary. So with the current code it crashes because it tries to initialize the metrics server after the second run. |
| func NewMetrics(addr string, logger logging.Logger) *Metrics { | ||
| registry := prometheus.NewRegistry() | ||
| registry.MustRegister(updateStakeAttempt, txRevertedTotal, operatorsUpdated) |
There was a problem hiding this comment.
Talked with @dmanc about this.
Feel like we're not addressing the proper separation of concern (absolutely my fault for this, just realized now). Created Layr-Labs/eigensdk-go#238 to do this in sdk as well.
But how are you guys registering the above metrics (operatorsUpdated, etc) with a separate registry?
I feel like ideal scenario would be that NewMetrics takes a registry as input, and then registers the variables against that registry. Maybe a hacky way that doesn't require too mcuh refactor is just to add a one-off function in this file registerPromVariables(reg prometheus.Registry) that just registers against some prom registry?
| @@ -35,12 +36,32 @@ var ( | |||
| }, []string{"quorum"}) | |||
| ) | |||
|
|
|||
| func StartMetricsServer(metricsAddr string) { | |||
| type Metrics struct { | |||
There was a problem hiding this comment.
See my below comment. Feel like we should rename this to MetricsServer. It should just take a registry as argument but not register the prometheus variables into it (since its just a MetricsServer). Ideally this metricsServer would live in sdk.
Then you would first call the registerPromVariables(reg prometheus.Registry) in main to register the metrics, and then start the webserver (if needed, maybe put that as flag in the binary).
|
Closed in favor of #38 |
Instead of creating a metrics server inside avssync, this change makes avssync constructor take the metrics server itself so that it can be passed in from the caller.