wip: refactor: uniquely identify metric time series by name and label sets #131
Conversation
| } | ||
|
|
||
| private enum MetricType { | ||
| case counterMetrics(String, MetricContainer<Counter>) |
There was a problem hiding this comment.
In general not sure yet about naming, ideas?
There was a problem hiding this comment.
Also I would love to benefit from your Swift expertise re below ... it's probably the Type? It should be typed out for each metric type?
note: consider making enum 'MetricType' conform to the 'Sendable' protocol
private enum MetricType {
There was a problem hiding this comment.
Yeah Type isn't right, this isn't a type, it's a container. Something feels off with the names here, this is a container but why do we need MetricContainer if it's a single field?
By what is this supposed to be keyed, MetricsByLabel or some MetricGroup maybe?
If you're seeing a note there's an associated warning (or error) that is the actual problem somewhere. Sendable is about concurrency safety, and yeah this may want to be Sendable if able to?
There was a problem hiding this comment.
A revised version of the names is ready for review. Let me know your thoughts? I’ve also addressed the Sendable error.
There was a problem hiding this comment.
Once the names are agreed upon, I propose implementing the changes for Gauge in this PR and then creating a separate PR for the rest.
| } | ||
|
|
||
| private enum MetricType { | ||
| case counterMetrics(String, MetricContainer<Counter>) |
There was a problem hiding this comment.
Yeah Type isn't right, this isn't a type, it's a container. Something feels off with the names here, this is a container but why do we need MetricContainer if it's a single field?
By what is this supposed to be keyed, MetricsByLabel or some MetricGroup maybe?
If you're seeing a note there's an associated warning (or error) that is the actual problem somewhere. Sendable is about concurrency safety, and yeah this may want to be Sendable if able to?
incertum
force-pushed
the
refactor/metrics-storage-enable-shared-names-same-type
branch
2 times, most recently
from
a3bb1e5 to
fd0e483
Compare
| private enum Metric { | ||
| case counter(Counter, help: String) | ||
| case counterWithLabels([String], [LabelsKey: Counter], help: String) | ||
| case counter(name: String, CounterGroup) |
There was a problem hiding this comment.
Isn't the name duplicated here now in both the box dictionary and then this enum case?
There was a problem hiding this comment.
It seemed that way before for labeled cases, so I thought it was a design preference.
Based on your comment, I understand that you approve of removing the small duplicative part. I’ve just re-pushed it.
Could you let me know if the design is now ok? If so, I’ll proceed to implement the changes for Gauge in this PR as well.
Previously, counters could only be identified by their name. This change allows multiple counters to share the same name, with each unique set of labels defining a distinct time series. Additionally, the internal data structure for a stored metric has been refactored, providing a more robust and programmatic representation. Signed-off-by: Melissa Kilby <[email protected]>
incertum
force-pushed
the
refactor/metrics-storage-enable-shared-names-same-type
branch
from
fd0e483 to
c697ee8
Compare
Previously, metrics were identified by their name. This change allows a shared metric name per metric type, with each unique set of labels defining a distinct time series.
Additionally, the internal data structure for a stored metric has been refactored, providing a more robust and programmatic representation.
CC @FranzBusch, @ktoso, @blindspotbounty
Let me know how you'd prefer to proceed. We have two main options:
Fixes #125 opened by @blindspotbounty