refactor[Histograms]: uniquely identify metric time series by name and label sets (2/2) #134
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…bel sets
Previously, {Value,Duration}Histograms could only be identified by their name. This change allows multiple {Value,Duration}Histograms to share the same name, with each unique set of labels defining a distinct time series.
However, the buckets are immutable for a MetricGroup once initialized with the first metric.
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-2
branch
from
0dadeef to
7808b75
Compare
ktoso
reviewed
Aug 26, 2025
| let help: String | ||
| } | ||
|
|
||
| private enum BucketType: Sendable, Hashable { |
There was a problem hiding this comment.
Doesn't feel like a "type" it is the actual buckets - plural because of collection of bucket which is one value in here
Suggested change
| private enum BucketType: Sendable, Hashable { | |
| private enum HistogramBuckets: Sendable, Hashable { |
ktoso
reviewed
Aug 26, 2025
| self.buckets = buckets | ||
| } | ||
|
|
||
| init(metricsByLabelSets: [LabelsKey: MetricWithHelp<Metric>] = [:]) { |
There was a problem hiding this comment.
i don't think we need this init; it can be just the previous one with buckets = nil
Co-authored-by: Konrad `ktoso` Malawski <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>
incertum
force-pushed
the
refactor/metrics-storage-enable-shared-names-same-type-2
branch
from
61dbf5b to
697b697
Compare
ktoso
reviewed
Aug 26, 2025
| histogram2.recordNanoseconds(150_000_000) // 150ms | ||
| histogram1.recordNanoseconds(1_500_000_000) // 1500ms | ||
| histogram0.recordNanoseconds(800_000_000) // 800ms | ||
| histogram2.recordNanoseconds(100_000_000) // 100ms |
There was a problem hiding this comment.
This is easier to write as
histogram.record(.milliseconds(100))
ktoso
added
🔨 semver/patch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refactor: uniquely identify {Value,Duration}Histograms by name and label sets
Previously, {Value,Duration}Histograms could only be identified by their name. This change allows multiple {Value,Duration}Histograms to share the same name, with each unique set of labels defining a distinct time series.
However, the buckets are immutable for a MetricGroup once initialized with the first metric.
Additionally, the internal data structure for a stored metric has been refactored, providing a more robust and programmatic representation.
CC @ktoso @FranzBusch
Follow for #131, fixes #125