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.

Sign up for GitHub

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

Jump to bottom

do not allocate gauge if cardinality metrics are turned off #246

Merged
merged 2 commits into from
Feb 16, 2024
Merged

do not allocate gauge if cardinality metrics are turned off #246

merged 2 commits into from
Feb 16, 2024

Conversation

brawndou
Copy link
Collaborator

@brawndou brawndou commented Feb 9, 2024

Changes:

  • Cached gauges will not be allocated if omitCardinalityMetrics is set to true

@codecov Codecov
Copy link

codecov bot commented Feb 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (83f9861) 48.04% compared to head (9fa1893) 48.04%.
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #246   +/-   ##
=======================================
  Coverage   48.04%   48.04%           
=======================================
  Files          65       65           
  Lines        6061     6061           
=======================================
  Hits         2912     2912           
  Misses       2709     2709           
  Partials      440      440

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VojtechVitek VojtechVitek mentioned this pull request Feb 14, 2024
Merged
@LukasJenicek
Copy link
Contributor

@brawndou

Thanks for the fast fix 👍. This was causing a panic in our go-chi telemetry middleware and I could not turn off the cardinality metrics easily.

panic: descriptor Desc{fqName: "tally.internal.counter_cardinality", help: "tally.internal.counter_cardinality gauge", constLabels: {}, variableLabels: {version,host,instance}} is invalid: "tally.internal.counter_cardinality" is not a valid metric name

goroutine 1 [running]:
github.com/uber-go/tally/v4/prometheus.NewReporter.func1({0x9be240, 0xc000226000})
	/home/lukas-jenicek/Projects/Go-Chi/telemetry/vendor/github.com/uber-go/tally/v4/prometheus/reporter.go:306 +0x5e
github.com/uber-go/tally/v4/prometheus.(*reporter).AllocateGauge(0xc00003a080, {0x91411d, 0x22}, 0xc0000a4390)
	/home/lukas-jenicek/Projects/Go-Chi/telemetry/vendor/github.com/uber-go/tally/v4/prometheus/reporter.go:415 +0x194
github.com/uber-go/tally/v4.newScopeRegistryWithShardCount(0xc000002180, 0xc0000a4240?, 0x0, 0x5?)
	/home/lukas-jenicek/Projects/Go-Chi/telemetry/vendor/github.com/uber-go/tally/v4/scope_registry.go:107 +0x654
github.com/uber-go/tally/v4.newRootScope({0xc0000a4240, {0x904b71, 0x4}, {0x0, 0x0}, {0x7f7b643f9008, 0xc00003a080}, {0x9bb9a8, 0x1}, {0x9c2e08, ...}, ...}, ...)
	/home/lukas-jenicek/Projects/Go-Chi/telemetry/vendor/github.com/uber-go/tally/v4/scope.go:192 +0x79b
github.com/uber-go/tally/v4.NewRootScope(...)
	/home/lukas-jenicek/Projects/Go-Chi/telemetry/vendor/github.com/uber-go/tally/v4/scope.go:118
github.com/go-chi/telemetry.newRootScope({0x0, {0x904b71, 0x4}, {0x0, 0x0}, {0x7f7b643f9008, 0xc00003a080}, {0x9bb9a8, 0x1}, {0x0, ...}, ...}, ...)

Do you know why the separator of the metrics name was changed from underscore to dot in the latest version v1.41.0 ?

SokolAndrey
SokolAndrey reviewed Feb 14, 2024
View reviewed changes
scope_registry.go
@@ -103,7 +103,7 @@ func newScopeRegistryWithShardCount(
}
r.subscopes[i].s[scopeRegistryKey(root.prefix, root.tags)] = root
}
if r.root.cachedReporter != nil {
if r.root.cachedReporter != nil && !omitCardinalityMetrics {
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls add unit test for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

SokolAndrey
SokolAndrey approved these changes Feb 16, 2024
View reviewed changes
Copy link
Collaborator

@SokolAndrey SokolAndrey left a comment

Choose a reason for hiding this comment

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

lgtm

scope_registry_test.go Outdated

func TestCachedReporterNoAllocOnOmitInternalMetrics(t *testing.T) {
r := newTestStatsReporter()
root, closer := NewRootScope(ScopeOptions{CachedReporter: r, OmitCardinalityMetrics: true}, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to check that it gets allocated when OmitCardinalityMetrics is false?

@brawndou brawndou merged commit e98f683 into uber-go:master Feb 16, 2024
7 checks passed
brawndou added a commit to brawndou/tally that referenced this pull request Feb 16, 2024
@brawndou
do not allocate gauge if cardinality metrics are turned off (uber-go#246
7eb95a9 
)

* do not allocate gauge if cardinality metrics are turned off

* add test cases
@brawndou brawndou mentioned this pull request Feb 17, 2024
Merged
brawndou added a commit that referenced this pull request Mar 6, 2024
@brawndou
do not allocate gauge if cardinality metrics are turned off (#246) (#248
3a29791 
)

* do not allocate gauge if cardinality metrics are turned off

* add test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SokolAndrey SokolAndrey SokolAndrey approved these changes

@shaan420 shaan420 Awaiting requested review from shaan420

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brawndou @LukasJenicek @SokolAndrey