[dvc][fc][tc] Lazily register client-side metrics #1104
Conversation
| @@ -109,14 +110,14 @@ public class IngestionStats { | |||
|
|
|||
| /** Record a version-level offset rewind events for VTs across all stores. */ | |||
| private final Count versionTopicEndOffsetRewindCount = new Count(); | |||
There was a problem hiding this comment.
Do we need to do this one too?
|
Is there a rhyme or reason to which metrics we did this fix for? I think it's non exhaustive? |
| @@ -207,6 +211,17 @@ public IngestionStats(VeniceServerConfig serverConfig) { | |||
| registerSensor(localMetricRepository, OFFSET_REGRESSION_DCR_ERROR, offsetRegressionDCRErrorSensor); | |||
| registerSensor(localMetricRepository, TOMBSTONE_CREATION_DCR, tombstoneCreationDCRSensor); | |||
| registerSensor(localMetricRepository, IDLE_TIME, idleTimeSensor); | |||
|
|
|||
| transformerLatencySensor = new WritePathLatencySensor(localMetricRepository, METRIC_CONFIG, TRANSFORMER_LATENCY); | |||
There was a problem hiding this comment.
I'm confused here. Why are some of these lazy and some not? When do these get registered?
There was a problem hiding this comment.
Some metrics are required to be registered by some unit/integration tests by asserting that their value is 0. Should I modify the tests to assert that the sensor be null instead?
The underlying sensor inside WritePathLatencySensor is registered as lazy, but when a record is called on it the sensor will be registered.
There was a problem hiding this comment.
It's ok to edit tests if the behavior no longer matches up. I think we should be uniform here unless theres a valid reason to not do so.
Summary
Unused metrics result in excessive WARN log messages. By registering metrics only when they are recorded, this issue is avoided.
How was this PR tested?
CI
Does this PR introduce any user-facing changes?