metrics: introduce CounterFloat64 and use for tenant RUs by dhartunian · Pull Request #96109 · cockroachdb/cockroach

dhartunian · 2023-01-27T18:15:54Z

Previously, we were prevented from using float64s directly in metric counters since counters were limited to ints. This led to the use of Gauges in situations where Counters would be preferable since we didn't have code to help manage a monotonically increasing float64 value.

This commit introduces some helpers around atomically adding float64s together and increasing one monotonically. Those primitives are composed into a CounterFloat64 type further used to construct AggCounterFloat64 which can be used in place of AggCounter to track per-tenant metrics.

The two GaugeFloat64 types used for tenant totalRU and totalKVRU metrics are replaced with the new CounterFloat64 type to properly reflect the fact that these are monotonically increasing values. This helps Prometheus when scraping these metrics to correctly account for missing data if necessary.

Resolves #68291

Epic: CRDB0-14536

Release note: None

cockroach-teamcity · 2023-01-27T18:16:03Z

This change is

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @andy-kimball, @dhartunian, and @tbg)

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go line 125 at r1 (raw file):

	// Report current consumption.
	metrics.totalRU.UpdateIfHigher(consumption.RU)

I think I'd prefer to let it update the value unconditionally.. If there's a bug (e.g. some kind of wraparound in an internal calculation), the IfHigher will hide what's going on (it will look like it's frozen)..

tbg

Having to Update a counter strikes me as weird. Counters are for counting! And we are counting. It's just not happening on the metric:

cockroach/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go

Lines 875 to 877 in ffc9606

    
           if c.shouldAccountForExternalIORUs() { 
        
           	c.mu.consumption.RU += float64(totalRU) 
        
           }

It's probably not worth spending too much time on pushing the metrics update down; after all that field is on a protobuf (which doesn't mean we can't use it anyway but still).

Re: Update checking, I'm fine either way, I think it is a judgment call. Ideally we don't use counters like we do here often but if we do hopefully we're using them only in incrementing fashions, so UpdateIfHigher seems mildly preferrable. Yes there could be a bug where we're putting a bogus number in and lock it forever, but there could also be concurrency bugs where we wipe out the correct number frequently. It's anyone's guess which one is more likely, both seem pretty unlikely.

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @abarganier, @andy-kimball, and @dhartunian)

pkg/util/metric/aggmetric/counter.go line 219 at r1 (raw file):

// Unlink disconnects this Counter from its parents. Unlike Gauge.Destroy, it
// does not decrement its value from its parent.

Would you mind matching this more closely to the (very recent) updated comment here?

cockroach/pkg/util/metric/aggmetric/gauge.go

Lines 230 to 238 in 7038e70

    
           // Unlink unlinks this child from the parent, i.e. the parent will no longer 
        
           // track this child (i.e. won't generate labels for it, etc). However, the child 
        
           // will continue to be functional and reference the parent, meaning updates to 
        
           // it will be reflected in the aggregate stored in the parent. 
        
           // 
        
           // See tenantrate.TestUseAfterRelease. 
        
           func (g *GaugeFloat64) Unlink() { 
        
           	g.parent.remove(g) 
        
           }

Code quote:

does not decrement its value from its parent

dhartunian

TFTRs. My preference is to keep UpdateIfHigher for now. The original request was to switch to counters for nicer Prometheus treatment and I think the code should enforce those expectations around the data since decrementing counters will cause downstream problems.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @abarganier and @andy-kimball)

pkg/util/metric/aggmetric/counter.go line 219 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Would you mind matching this more closely to the (very recent) updated comment here?

cockroach/pkg/util/metric/aggmetric/gauge.go

Lines 230 to 238 in 7038e70

// Unlink unlinks this child from the parent, i.e. the parent will no longer

// track this child (i.e. won't generate labels for it, etc). However, the child

// will continue to be functional and reference the parent, meaning updates to

// it will be reflected in the aggregate stored in the parent.

//

// See tenantrate.TestUseAfterRelease.

func (g *GaugeFloat64) Unlink() {

g.parent.remove(g)

}

Done.

Previously, we were prevented from using float64s directly in metric counters since counters were limited to ints. This led to the use of Gauges in situations where Counters would be preferable since we didn't have code to help manage a monotonically increasing float64 value. This commit introduces some helpers around atomically adding float64s together and increasing one monotonically. Those primitives are composed into a `CounterFloat64` type further used to construct `AggCounterFloat64` which can be used in place of `AggCounter` to track per-tenant metrics. The two `GaugeFloat64` types used for tenant `totalRU` and `totalKVRU` metrics are replaced with the new `CounterFloat64` type to properly reflect the fact that these are monotonically increasing values. This helps Prometheus when scraping these metrics to correctly account for missing data if necessary. Resolves cockroachdb#68291 Epic: CRDB0-14536 Release note: None

dhartunian · 2023-02-09T19:51:40Z

bors r=RaduBerinde,tbg

craig · 2023-02-09T20:58:38Z

Build succeeded:

Bazel Essential CI (Cockroach)

PR cockroachdb#96109 failed to amend the hdr test files. Resolves cockroachdb#96909 Epic: None Release note: None

96921: metrics: fix agg metrics hdr test files r=dt a=dhartunian PR #96109 failed to amend the hdr test files. Resolves #96909 Epic: None Release note: None Co-authored-by: David Hartunian <davidh@cockroachlabs.com>

dhartunian requested review from RaduBerinde, abarganier and andy-kimball January 27, 2023 18:16

dhartunian marked this pull request as ready for review January 27, 2023 18:16

dhartunian requested a review from a team January 27, 2023 18:16

dhartunian requested a review from a team as a code owner January 27, 2023 18:16

dhartunian force-pushed the add-counterfloat64-for-tenant-metrics branch from a732fae to 5c6944b Compare January 27, 2023 18:18

dhartunian requested a review from tbg January 27, 2023 18:18

RaduBerinde approved these changes Jan 27, 2023

View reviewed changes

tbg approved these changes Jan 30, 2023

View reviewed changes

dhartunian force-pushed the add-counterfloat64-for-tenant-metrics branch from 5c6944b to 11597cb Compare February 7, 2023 19:32

dhartunian commented Feb 7, 2023

View reviewed changes

dhartunian force-pushed the add-counterfloat64-for-tenant-metrics branch from 11597cb to e963f97 Compare February 8, 2023 22:05

dhartunian force-pushed the add-counterfloat64-for-tenant-metrics branch from e963f97 to f96b21f Compare February 9, 2023 16:17

craig bot merged commit faeebee into cockroachdb:master Feb 9, 2023

dhartunian added a commit to dhartunian/cockroach that referenced this pull request Feb 10, 2023

metrics: fix agg metrics hdr test files

b2da2b5

PR cockroachdb#96109 failed to amend the hdr test files. Resolves cockroachdb#96909 Epic: None Release note: None

dhartunian mentioned this pull request Feb 10, 2023

metrics: fix agg metrics hdr test files #96921

Merged

dhartunian deleted the add-counterfloat64-for-tenant-metrics branch February 5, 2024 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metrics: introduce CounterFloat64 and use for tenant RUs#96109

metrics: introduce CounterFloat64 and use for tenant RUs#96109
craig[bot] merged 1 commit intocockroachdb:masterfrom
dhartunian:add-counterfloat64-for-tenant-metrics

dhartunian commented Jan 27, 2023

Uh oh!

cockroach-teamcity commented Jan 27, 2023

Uh oh!

RaduBerinde left a comment

Uh oh!

tbg left a comment

Uh oh!

dhartunian left a comment

Uh oh!

dhartunian commented Feb 9, 2023

Uh oh!

craig bot commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if c.shouldAccountForExternalIORUs() {
	c.mu.consumption.RU += float64(totalRU)
	}

	// Unlink unlinks this child from the parent, i.e. the parent will no longer
	// track this child (i.e. won't generate labels for it, etc). However, the child
	// will continue to be functional and reference the parent, meaning updates to
	// it will be reflected in the aggregate stored in the parent.
	//
	// See tenantrate.TestUseAfterRelease.
	func (g *GaugeFloat64) Unlink() {
	g.parent.remove(g)
	}

Conversation

dhartunian commented Jan 27, 2023

Uh oh!

cockroach-teamcity commented Jan 27, 2023

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian commented Feb 9, 2023

Uh oh!

craig bot commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants