Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CardinalityCounter to testmertics #209

Merged
merged 3 commits into from May 9, 2024

Conversation

Vamsi-Mundra
Copy link
Contributor

@Vamsi-Mundra Vamsi-Mundra commented May 8, 2024

Reasons For the Change:

While we are using testmetrics pkg in one of our unit tests, we observed CheckCardinalityCounter is not working as expected.
Inside the Provider of testmetrics, all the counters we are using, are of metrics.go except for cardCounters. For cardCounters we are using xmetrics.HLLCounter. We weren't adding labels properly and CheckCardinalityCounter was not taking labelValues. Also all the counters of metrics.go are having Provider of testmetrics which is not for xmetrics.HLLCounter. We wanted to add new CardinalityCounter inside metrics.go of testmetrics that has Provider inside it and adds, checks for labels correctly.

Changes:

  • Added CardinalityCounter which implements CardinalityCounter interface of go-kit/metrics
  • Updated Provider to use CardinalityCounter of testmetrics

@Vamsi-Mundra Vamsi-Mundra requested review from ypaq and slizco May 8, 2024 21:24
Copy link
Contributor

@ypaq ypaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments, but looks good overall.

go-kit/metrics/testmetrics/provider.go Outdated Show resolved Hide resolved
go-kit/metrics/testmetrics/metrics.go Outdated Show resolved Hide resolved
go-kit/metrics/testmetrics/metrics.go Outdated Show resolved Hide resolved
@Vamsi-Mundra Vamsi-Mundra requested a review from ypaq May 8, 2024 22:32
Copy link
Contributor

@ypaq ypaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Vamsi-Mundra Vamsi-Mundra merged commit 35653eb into vmc-update-testmetrics May 9, 2024
3 checks passed
@Vamsi-Mundra Vamsi-Mundra deleted the vmc-update-provider branch May 9, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants