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

fix broken internal metrics names and make them optional #203

Merged
merged 4 commits into from
Jan 26, 2023
Merged

fix broken internal metrics names and make them optional #203

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
764e6b1
removing spammy message
brawndou Jan 25, 2023
0c93fed
improved internal metrics impl
brawndou Jan 26, 2023
6c7200b
Merge branch 'uber-go:v3' into v3
brawndou Jan 26, 2023
d1e0b4e
internal metric names are sanitized
brawndou Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions scope.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2022 Uber Technologies, Inc.
// Copyright (c) 2023 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -28,7 +28,13 @@ import (
"go.uber.org/atomic"
)

type InternalMetricOption int

const (
Unset InternalMetricOption = iota
SendInternalMetrics
OmitInternalMetrics

_defaultInitialSliceSize = 16
)

Expand Down Expand Up @@ -95,15 +101,15 @@ type scope struct {

// ScopeOptions is a set of options to construct a scope.
type ScopeOptions struct {
Tags map[string]string
Prefix string
Reporter StatsReporter
CachedReporter CachedStatsReporter
Separator string
DefaultBuckets Buckets
SanitizeOptions *SanitizeOptions
registryShardCount uint
skipInternalMetrics bool
Tags map[string]string
Prefix string
Reporter StatsReporter
CachedReporter CachedStatsReporter
Separator string
DefaultBuckets Buckets
SanitizeOptions *SanitizeOptions
registryShardCount uint
internalMetricsOption InternalMetricOption
}

// NewRootScope creates a new root Scope with a set of options and
Expand Down Expand Up @@ -173,7 +179,7 @@ func newRootScope(opts ScopeOptions, interval time.Duration) *scope {
s.tags = s.copyAndSanitizeMap(opts.Tags)

// Register the root scope
s.registry = newScopeRegistryWithShardCount(s, opts.registryShardCount, opts.skipInternalMetrics)
s.registry = newScopeRegistryWithShardCount(s, opts.registryShardCount, opts.internalMetricsOption)

if interval > 0 {
s.wg.Add(1)
Expand Down
26 changes: 15 additions & 11 deletions scope_registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2022 Uber Technologies, Inc.
// Copyright (c) 2023 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -34,9 +34,9 @@ var (

// Metrics related.
internalTags = map[string]string{"version": Version}
counterCardinalityName = "tally.internal.counter-cardinality"
gaugeCardinalityName = "tally.internal.gauge-cardinality"
histogramCardinalityName = "tally.internal.histogram-cardinality"
counterCardinalityName = "tally-internal-counter-cardinality"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact that this can cause a panic is not great. We should handle an invalid metric names gracefully. We could add sanitazation to the code where the metric is being registered. Though it probably makes sense to have it as a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

created #204 to track this request

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think either dots or dashes are accepted by prometheus reporter. At least they not legal characters according to Prom data model

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will change to underscore

gaugeCardinalityName = "tally-internal-gauge-cardinality"
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we please have a test for that?
funny enough we have a comment in Prom reporter example about dashes and dots
https://github.com/uber-go/tally/blob/master/prometheus/example/prometheus_main.go#L37

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll have the scope's sanitizer process it during runtime instead. This way we can be agnostic about whatever scope/reporter/sanitizer is underneath.

histogramCardinalityName = "tally-internal-histogram-cardinality"
)

type scopeRegistry struct {
Expand All @@ -45,24 +45,28 @@ type scopeRegistry struct {
// We need a subscope per GOPROC so that we can take advantage of all the cpu available to the application.
subscopes []*scopeBucket
// Toggles internal metrics reporting.
skipInternalMetrics bool
internalMetricsOption InternalMetricOption
}

type scopeBucket struct {
mu sync.RWMutex
s map[string]*scope
}

func newScopeRegistryWithShardCount(root *scope, shardCount uint, skipInternalMetrics bool) *scopeRegistry {
func newScopeRegistryWithShardCount(
root *scope,
shardCount uint,
internalMetricsOption InternalMetricOption,
) *scopeRegistry {
if shardCount == 0 {
shardCount = uint(runtime.GOMAXPROCS(-1))
}

r := &scopeRegistry{
root: root,
subscopes: make([]*scopeBucket, shardCount),
seed: maphash.MakeSeed(),
skipInternalMetrics: skipInternalMetrics,
root: root,
subscopes: make([]*scopeBucket, shardCount),
seed: maphash.MakeSeed(),
internalMetricsOption: internalMetricsOption,
}
for i := uint(0); i < shardCount; i++ {
r.subscopes[i] = &scopeBucket{
Expand Down Expand Up @@ -227,7 +231,7 @@ func (r *scopeRegistry) removeWithRLock(subscopeBucket *scopeBucket, key string)

// Records internal Metrics' cardinalities.
func (r *scopeRegistry) reportInternalMetrics() {
if r.skipInternalMetrics {
if r.internalMetricsOption != SendInternalMetrics {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can eventually set a default option if it's unset

return
}

Expand Down
6 changes: 3 additions & 3 deletions scope_registry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2022 Uber Technologies, Inc.
// Copyright (c) 2023 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -55,7 +55,7 @@ func TestVerifyCachedTaggedScopesAlloc(t *testing.T) {

func TestNewTestStatsReporterOneScope(t *testing.T) {
r := newTestStatsReporter()
root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: false}, 0)
root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: SendInternalMetrics}, 0)
s := root.(*scope)

numFakeCounters := 3
Expand Down Expand Up @@ -101,7 +101,7 @@ func TestNewTestStatsReporterOneScope(t *testing.T) {

func TestNewTestStatsReporterManyScopes(t *testing.T) {
r := newTestStatsReporter()
root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: false}, 0)
root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: SendInternalMetrics}, 0)
wantCounters, wantGauges, wantHistograms := int64(3), int64(2), int64(1)

s := root.(*scope)
Expand Down
92 changes: 54 additions & 38 deletions scope_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) 2021 Uber Technologies, Inc.
// Copyright (c) 2023 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -353,7 +353,7 @@ func (r *testStatsReporter) Flush() {

func TestWriteTimerImmediately(t *testing.T) {
r := newTestStatsReporter()
s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0)
s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
defer closer.Close()
r.tg.Add(1)
s.Timer("ticky").Record(time.Millisecond * 175)
Expand All @@ -362,7 +362,7 @@ func TestWriteTimerImmediately(t *testing.T) {

func TestWriteTimerClosureImmediately(t *testing.T) {
r := newTestStatsReporter()
s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0)
s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
defer closer.Close()
r.tg.Add(1)
tm := s.Timer("ticky")
Expand All @@ -372,7 +372,7 @@ func TestWriteTimerClosureImmediately(t *testing.T) {

func TestWriteReportLoop(t *testing.T) {
r := newTestStatsReporter()
s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 10)
s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 10)
defer closer.Close()

r.cg.Add(1)
Expand All @@ -390,7 +390,7 @@ func TestWriteReportLoop(t *testing.T) {

func TestCachedReportLoop(t *testing.T) {
r := newTestStatsReporter()
s, closer := NewRootScope(ScopeOptions{CachedReporter: r, skipInternalMetrics: true}, 10)
s, closer := NewRootScope(ScopeOptions{CachedReporter: r, internalMetricsOption: OmitInternalMetrics}, 10)
defer closer.Close()

r.cg.Add(1)
Expand All @@ -408,9 +408,9 @@ func TestCachedReportLoop(t *testing.T) {
func testReportLoopFlushOnce(t *testing.T, cached bool) {
r := newTestStatsReporter()

scopeOpts := ScopeOptions{CachedReporter: r, skipInternalMetrics: true}
scopeOpts := ScopeOptions{CachedReporter: r, internalMetricsOption: OmitInternalMetrics}
if !cached {
scopeOpts = ScopeOptions{Reporter: r, skipInternalMetrics: true}
scopeOpts = ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}
}

s, closer := NewRootScope(scopeOpts, 10*time.Minute)
Expand Down Expand Up @@ -448,7 +448,7 @@ func TestReporterFlushOnce(t *testing.T) {
func TestWriteOnce(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
defer closer.Close()

s := root.(*scope)
Expand Down Expand Up @@ -519,10 +519,10 @@ func TestHistogramSharedBucketMetrics(t *testing.T) {
var (
r = newTestStatsReporter()
scope = newRootScope(ScopeOptions{
Prefix: "",
Tags: nil,
CachedReporter: r,
skipInternalMetrics: true,
Prefix: "",
Tags: nil,
CachedReporter: r,
internalMetricsOption: OmitInternalMetrics,
}, 0)
builder = func(s Scope) func(map[string]string) {
buckets := MustMakeLinearValueBuckets(10, 10, 3)
Expand Down Expand Up @@ -591,10 +591,10 @@ func TestConcurrentUpdates(t *testing.T) {
counterIncrs = 5000
rs = newRootScope(
ScopeOptions{
Prefix: "",
Tags: nil,
CachedReporter: r,
skipInternalMetrics: true,
Prefix: "",
Tags: nil,
CachedReporter: r,
internalMetricsOption: OmitInternalMetrics,
}, 0,
)
scopes = []Scope{rs}
Expand Down Expand Up @@ -640,9 +640,9 @@ func TestCounterSanitized(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{
Reporter: r,
SanitizeOptions: &alphanumericSanitizerOpts,
skipInternalMetrics: true,
Reporter: r,
SanitizeOptions: &alphanumericSanitizerOpts,
internalMetricsOption: OmitInternalMetrics,
}, 0)
defer closer.Close()

Expand Down Expand Up @@ -698,7 +698,7 @@ func TestCounterSanitized(t *testing.T) {
func TestCachedReporter(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{CachedReporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(ScopeOptions{CachedReporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
defer closer.Close()

s := root.(*scope)
Expand Down Expand Up @@ -735,7 +735,7 @@ func TestCachedReporter(t *testing.T) {
func TestRootScopeWithoutPrefix(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
defer closer.Close()

s := root.(*scope)
Expand Down Expand Up @@ -769,7 +769,9 @@ func TestRootScopeWithoutPrefix(t *testing.T) {
func TestRootScopeWithPrefix(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(
ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0,
)
defer closer.Close()

s := root.(*scope)
Expand Down Expand Up @@ -803,7 +805,11 @@ func TestRootScopeWithPrefix(t *testing.T) {
func TestRootScopeWithDifferentSeparator(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Separator: "_", Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(
ScopeOptions{
Prefix: "foo", Separator: "_", Reporter: r, internalMetricsOption: OmitInternalMetrics,
}, 0,
)
defer closer.Close()

s := root.(*scope)
Expand Down Expand Up @@ -837,7 +843,9 @@ func TestRootScopeWithDifferentSeparator(t *testing.T) {
func TestSubScope(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(
ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0,
)
defer closer.Close()

tags := map[string]string{"foo": "bar"}
Expand Down Expand Up @@ -879,7 +887,7 @@ func TestSubScope(t *testing.T) {
func TestSubScopeClose(t *testing.T) {
r := newTestStatsReporter()

rs, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, skipInternalMetrics: true}, 0)
rs, closer := NewRootScope(ScopeOptions{Prefix: "foo", Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
// defer closer.Close()
_ = closer

Expand Down Expand Up @@ -970,7 +978,11 @@ func TestTaggedSubScope(t *testing.T) {
r := newTestStatsReporter()

ts := map[string]string{"env": "test"}
root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Tags: ts, Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(
ScopeOptions{
Prefix: "foo", Tags: ts, Reporter: r, internalMetricsOption: OmitInternalMetrics,
}, 0,
)
defer closer.Close()

s := root.(*scope)
Expand Down Expand Up @@ -1022,11 +1034,11 @@ func TestTaggedSanitizedSubScope(t *testing.T) {

ts := map[string]string{"env": "test:env"}
root, closer := NewRootScope(ScopeOptions{
Prefix: "foo",
Tags: ts,
Reporter: r,
SanitizeOptions: &alphanumericSanitizerOpts,
skipInternalMetrics: true,
Prefix: "foo",
Tags: ts,
Reporter: r,
SanitizeOptions: &alphanumericSanitizerOpts,
internalMetricsOption: OmitInternalMetrics,
}, 0)
defer closer.Close()
s := root.(*scope)
Expand Down Expand Up @@ -1055,7 +1067,11 @@ func TestTaggedExistingReturnsSameScope(t *testing.T) {
nil,
{"env": "test"},
} {
root, closer := NewRootScope(ScopeOptions{Prefix: "foo", Tags: initialTags, Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(
ScopeOptions{
Prefix: "foo", Tags: initialTags, Reporter: r, internalMetricsOption: OmitInternalMetrics,
}, 0,
)
defer closer.Close()

rootScope := root.(*scope)
Expand Down Expand Up @@ -1132,7 +1148,7 @@ func TestSnapshot(t *testing.T) {

func TestCapabilities(t *testing.T) {
r := newTestStatsReporter()
s, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0)
s, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
defer closer.Close()
assert.True(t, s.Capabilities().Reporting())
assert.False(t, s.Capabilities().Tagging())
Expand Down Expand Up @@ -1160,8 +1176,8 @@ func TestScopeDefaultBuckets(t *testing.T) {
90 * time.Millisecond,
120 * time.Millisecond,
},
Reporter: r,
skipInternalMetrics: true,
Reporter: r,
internalMetricsOption: OmitInternalMetrics,
}, 0)
defer closer.Close()

Expand Down Expand Up @@ -1192,7 +1208,7 @@ func newTestMets(scope Scope) testMets {
func TestReturnByValue(t *testing.T) {
r := newTestStatsReporter()

root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)
defer closer.Close()

s := root.(*scope)
Expand All @@ -1209,7 +1225,7 @@ func TestReturnByValue(t *testing.T) {

func TestScopeAvoidReportLoopRunOnClose(t *testing.T) {
r := newTestStatsReporter()
root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, 0)
root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, 0)

s := root.(*scope)
s.reportLoopRun()
Expand All @@ -1224,7 +1240,7 @@ func TestScopeAvoidReportLoopRunOnClose(t *testing.T) {

func TestScopeFlushOnClose(t *testing.T) {
r := newTestStatsReporter()
root, closer := NewRootScope(ScopeOptions{Reporter: r, skipInternalMetrics: true}, time.Hour)
root, closer := NewRootScope(ScopeOptions{Reporter: r, internalMetricsOption: OmitInternalMetrics}, time.Hour)

r.cg.Add(1)
root.Counter("foo").Inc(1)
Expand Down