metrics: refactor functional gauge to accept gauge value in callback#163499
metrics: refactor functional gauge to accept gauge value in callback#163499kyle-a-wong wants to merge 1 commit intocockroachdb:masterfrom
Conversation
kyle-a-wong
requested review from
angles-n-daemons and
arjunmahishi
and removed request for
a team
|
Merging to
|
|
This change is |
dhartunian
left a comment
dhartunian
left a comment
There was a problem hiding this comment.
@dhartunian made 2 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aerfrei, @angles-n-daemons, @arjunmahishi, and @kyle-a-wong).
-- commits line 7 at r1:
Can you link to the epic and other work to explain the motivation for this?
There's a bit of confusion that this introduces about whether Inc/Dec/Update do anything in a Functional Gauge. I wonder if we should have two different kinds of functional gauges. Maybe FunctionalGauge and DerivedGauge? The new API is now more subtle and error-prone.
pkg/util/metric/aggmetric/gauge.go line 47 at r1 (raw file):
) *AggGauge { g := &AggGauge{} gaugeFn := func(_ int64) int64 {
Hmm I guess the existing API was confusing too...So an aggregate functional gauge is derived from values directly, but it's a function of all the children, not the internal value of the aggregate. This is confusing. Maybe we should rethink the API/implementation or at the very least carefully document the behavior. Calling Inc/Dec/Update on a functional AggGauge will not do anything.
kyle-a-wong
force-pushed
the
update_functional_gauge
branch
from
d6e99ab to
2aded15
Compare
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
github-actions
bot
added
the
o-AI-Review-Potential-Issue-Detected
kyle-a-wong
force-pushed
the
update_functional_gauge
branch
4 times, most recently
from
31266b5 to
cb88536
Compare
Defines a new DerivedGauge that is a special implementation of Gauge that computes a derived gauge value, based on the call back used to make the DerivedGauge and the last updated value of the gauge. This replaces the old "Functional Gauge" implentation. Renames the agg gauge functional gauge to "derived gauge" to align with the new derived gauge type. Defines a new DerivedGaugeVec type that is the Prometheus vector equivalent of DerivedGauge. Defines a new FunctionalGauge type, that calls the provided callback to compute the gauges value. By defining these as concrete types, the API of each type becomes more clear and sensible. For example, the previous "functional gauge" was just normal gauge, This gauge has Update, Inc, Dec methods, but they didn't do anything to the gauge's reported value. Epic: None Release note: None
kyle-a-wong
force-pushed
the
update_functional_gauge
branch
from
cb88536 to
443394a
Compare
3 participants
Refactors functional gauges to take the gauges
current value as an argument. Now, functional
gauges can be used with the
Update,Inc, andDecmethods if desiredEpic: None
Release note: None