wip: refactor: uniquely identify metric time series by name and label sets #131
Conversation
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]>
| } | ||
|
|
||
| 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?
| private enum Metric { | ||
| case counter(Counter, help: String) | ||
| case counterWithLabels([String], [LabelsKey: Counter], help: String) | ||
| private typealias Metric<T> = (metric: T, help: String) |
There was a problem hiding this comment.
I'd avoid tuples when a thing seems more like a normal data type, make it a real type I think
| } | ||
|
|
||
| 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?
| guard let _ = MetricContainer.metrics.first?.value else { | ||
| continue | ||
| } | ||
| for Metric in MetricContainer.metrics.values { |
There was a problem hiding this comment.
| for Metric in MetricContainer.metrics.values { | |
| for metric in MetricContainer.metrics.values { |
| buffer.addLine(prefix: prefixType, name: label, value: "counter") | ||
| for counter in counters.values { | ||
| counter.emit(into: &buffer) | ||
| case .counterMetrics(_, let MetricContainer): |
There was a problem hiding this comment.
Not sure why the uppercase names?
| case .counterMetrics(_, let MetricContainer): | |
| case .counterMetrics(_, let metricContainer): |
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