Register database custom metrics using a multi gauge #86
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
============================================
- Coverage 68.25% 66.40% -1.86%
Complexity 106 106
============================================
Files 15 17 +2
Lines 608 625 +17
Branches 32 32
============================================
Hits 415 415
- Misses 174 191 +17
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...n/java/org/hypertrace/core/serviceframework/docstore/metrics/model/GaugeBuilderProvider.java
Outdated
Show resolved
Hide resolved
suresh-prakash
changed the title
...rics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java
Outdated
Show resolved
Hide resolved
...rics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java
Outdated
Show resolved
Hide resolved
…ework/metrics/PlatformMetricsRegistry.java
...rics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java
Outdated
Show resolved
Hide resolved
...rics/src/main/java/org/hypertrace/core/serviceframework/metrics/PlatformMetricsRegistry.java
Outdated
Show resolved
Hide resolved
| isInit = false; | ||
| } | ||
|
|
||
| static MultiGauge registerMultiGauge(final String gaugeName) { |
There was a problem hiding this comment.
I would still suggest flipping this around to match the other meters. The Registry is what external code interacts with today, which returns a meter that is then used to report.
public static ResizableGauge registerResizableGauge(
String name // consider adding tags so we don't have to break the API later)
That would mean you'd remove the registration from the constructor of ResizableGauge (and make it package private).
If you want to switch that around (since I think constructing a meter and registering it separately is a better design, also what micrometer does), we could do (it would be inconsistent, so we'd want to eventually add support to flip the other meters, too) so but I would still keep the registration out of the ResizableGauge constructor and I would make this function in platform metrics registry generic (e.g. <T extends Registrable> T register(T registrable))
Hope that makes sense, happy to discuss if not.
There was a problem hiding this comment.
Makes sense. Picking the first approach here for consistency.
consider adding tags so we don't have to break the API later)
We may not have the labels/tags upfront while registering the gauge. We only get them from the query results. Did you mean taking in any additional Tags and passing down empty Tags from monitorCustomMetrics?
There was a problem hiding this comment.
We may not have the labels/tags upfront while registering the gauge. We only get them from the query results. Did you mean taking in any additional Tags and passing down empty Tags from monitorCustomMetrics?
Yeah, I think the underlying multigauge allows both tags up front (presumably static things, like what service this is) and tags on the individual measurement reports. I suppose we could always overload it later if we don't need it now.
Using MultiGauge to register database custom metrics so that the old tags stop reporting in subsequent runs if the database doesn't yield the results.