Migrated metrics to prometheus #434
Conversation
| sample.stop(meterRegistry.timer("snapshot-factory.new-snapshot.time")) | ||
| sample.stop( | ||
| meterRegistry.timer( | ||
| "snapshot-factory.seconds", |
There was a problem hiding this comment.
maybe snapshot_factory to be consistent with other metrics?
There was a problem hiding this comment.
yea, I was looking at some our other metrics, but looks like this way would be better, I changed to dots everywhere
| groupSample.stop(meterRegistry.timer("snapshot-factory.get-snapshot-for-group.time")) | ||
| groupSample.stop( | ||
| meterRegistry.timer( | ||
| "snapshot-factory.seconds", |
There was a problem hiding this comment.
same here - snapshot_factory for consistency
| .checkpoint("snapshot-updater-groups-published") | ||
| .name("snapshot-updater-groups-published").metrics() | ||
| .name("snapshot-updater.count.total") |
There was a problem hiding this comment.
snapshot_updater/snapshot.updater - . is mapped to _, right?
There was a problem hiding this comment.
. is, - isn't
| .record( | ||
| stopTimer - startTimer, | ||
| TimeUnit.MILLISECONDS | ||
| TimeUnit.SECONDS |
There was a problem hiding this comment.
prometheus has some naming conventions and default units that are recommended to be used for consistency, so according to them it's better to use seconds
...pl/allegro/tech/servicemesh/envoycontrol/server/callbacks/MetricsDiscoveryServerCallbacks.kt
Outdated
Show resolved
Hide resolved
...pl/allegro/tech/servicemesh/envoycontrol/server/callbacks/MetricsDiscoveryServerCallbacks.kt
Outdated
Show resolved
Hide resolved
| .name("snapshot-updater-merged").metrics() | ||
| .name("snapshot.updater.count.total") | ||
| .tag("status", "merged") | ||
| .tag("type", "global") |
There was a problem hiding this comment.
so as there were separate metrics for groups and global snapshot-updater I introduced it as a type label, so this type is like "snapshot-type"
Co-authored-by: kozjan <[email protected]>
| meterRegistry.gauge(metricName, Tags.of("status", "instance-changed"), it.instanceChanges) | ||
| meterRegistry.gauge(metricName, Tags.of("status", "snapshot-changed"), it.snapshotChanges) | ||
| meterRegistry.gauge("cache.groups.count", it.cacheGroupsCount) | ||
| it.meterRegistry.more().counter("services.watch.errors.total", listOf(), it.errorWatchingServices) |
There was a problem hiding this comment.
I think it would be better to have consistent name. So if we have watched-services, then it should be watched-services.errors.total
| }, 0, interval, TimeUnit.SECONDS) | ||
| }, FluxSink.OverflowStrategy.LATEST) | ||
| return aclFlux.doOnCancel { | ||
| meterRegistry.counter("cross-dc-synchronization.cancelled").increment() | ||
| meterRegistry.counter("cross.dc.synchronization.cancelled").increment() |
There was a problem hiding this comment.
Why there is not total at the end of metric name?
| @@ -226,10 +226,10 @@ class ConsulServiceChanges( | |||
| if (ready) { | |||
| val stopTimer = System.currentTimeMillis() | |||
| readinessStateHandler.ready() | |||
| metrics.meterRegistry.timer("envoy-control.warmup.time") | |||
| metrics.meterRegistry.timer("envoy-control.warmup.seconds") | |||
There was a problem hiding this comment.
Both - and . will be replaced by _ in final metric name. So I suggest to use _ as separator in all metrics names instead - and .
No description provided.