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

Report Tag cache size as part of internal metrics #189

Merged
merged 25 commits into from
Jan 13, 2023

Conversation

brawndou
Copy link
Collaborator

This helps service owners know if they are causing Tally to use too much memory

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #189 (05764ef) into master (2dbfcc6) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   47.46%   47.78%   +0.32%     
==========================================
  Files          64       64              
  Lines        5948     5985      +37     
==========================================
+ Hits         2823     2860      +37     
  Misses       2685     2685              
  Partials      440      440              
Impacted Files Coverage Δ
internal/cache/tag_cache.go 89.47% <100.00%> (+1.97%) ⬆️
m3/reporter.go 92.87% <100.00%> (+0.03%) ⬆️
scope.go 95.01% <100.00%> (ø)
scope_registry.go 96.66% <100.00%> (+1.21%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@mway mway left a comment

Choose a reason for hiding this comment

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

General note: the tag count is not the same as cardinality; to get a correct cardinality count you will need to count every root+subscope's metrics.

If we just use the number of tag sets, it misses the (variable) coefficient of how many metrics use each of them.

m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter.go Outdated Show resolved Hide resolved
m3/reporter_test.go Outdated Show resolved Hide resolved
@brawndou brawndou marked this pull request as ready for review January 6, 2023 01:24
m3/reporter.go Show resolved Hide resolved
m3/reporter.go Show resolved Hide resolved
m3/reporter_integration_test.go Show resolved Hide resolved
m3/reporter_test.go Outdated Show resolved Hide resolved
scope_registry.go Show resolved Hide resolved
scope_registry.go Outdated Show resolved Hide resolved
@brawndou brawndou requested review from mway and SokolAndrey and removed request for SokolAndrey January 9, 2023 23:12
@brawndou brawndou merged commit c347bbf into uber-go:master Jan 13, 2023
brawndou added a commit to brawndou/tally that referenced this pull request Jan 14, 2023
Additional internal metrics will be emitted:

- `tally.internal.num-tag-cache` will report the number of tags
- `tally.internal.counter-cardinality` will report the number of counters across all scopes
- `tally.internal.gauge-cardinality` will report the number of gauges across all scopes
- `tally.internal.histogram-cardinality` will report the number of histograms across all scopes

Other changes:
- refactored tests to use internal metric counts defined in variables
brawndou added a commit that referenced this pull request Jan 14, 2023
Additional internal metrics will be emitted:

- `tally.internal.num-tag-cache` will report the number of tags
- `tally.internal.counter-cardinality` will report the number of counters across all scopes
- `tally.internal.gauge-cardinality` will report the number of gauges across all scopes
- `tally.internal.histogram-cardinality` will report the number of histograms across all scopes

Other changes:
- refactored tests to use internal metric counts defined in variables
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.

4 participants