This repository was archived by the owner on Jul 31, 2023. It is now read-only.
Fix const labels with derived metrics#1221
Merged
james-bebbington merged 1 commit intocensus-instrumentation:masterfrom Jul 19, 2020
Merged
Fix const labels with derived metrics#1221james-bebbington merged 1 commit intocensus-instrumentation:masterfrom
james-bebbington merged 1 commit intocensus-instrumentation:masterfrom
Conversation
Include the const labels in `baseMetric.upsertEntry` in the same way as `baseMetric.entryForValues`. The "Derived" form of metrics use an `UpsertEntry` method to provide the function that supplies the metric value as well as the label values. These methods use `baseMetric.upsertEntry` to do the work, but that method was ignoring any const labels set by `WithConstLabel`. Commit c31d268 added const labels to metrics and updated `baseMetric.entryForValues` to add the const labels, but `baseMetric.upsertEntry` was not similarly updated. This cause `UpsertEntry` to return an `errKeyValueMismatch` error ("must supply the same number of label values as keys used to construct this metric") when called unless the values for the const labels were passed to `UpsertEntry`. That kind of defeats the purpose of const labels. Add a test case for const labels on Int64DerivedGauge. It is not necessary to add test cases for all the derived metrics as they all use the same `baseMetric` implementation.
james-bebbington
approved these changes
Jul 19, 2020
Collaborator
james-bebbington
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Include the const labels in
baseMetric.upsertEntryin the same way asbaseMetric.entryForValues.The "Derived" form of metrics use an
UpsertEntrymethod to provide the function that supplies the metric value as well as the label values. These methods usebaseMetric.upsertEntryto do the work, but that method was ignoring any const labels set byWithConstLabel.Commit c31d268 added const labels to metrics and updated
baseMetric.entryForValuesto add the const labels, butbaseMetric.upsertEntrywas not similarly updated.This causes
UpsertEntryto return anerrKeyValueMismatcherror ("must supply the same number of label values as keys used to construct this metric") when called unless the values for the const labels were passed toUpsertEntry. That kind of defeats the purpose of const labels.Add a test case for const labels on Int64DerivedGauge. It is not necessary to add test cases for all the derived metrics as they all use the same
baseMetricimplementation.