-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
admission: introduce elastic io token exhausted duration metric #124078
admission: introduce elastic io token exhausted duration metric #124078
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
c2271b4
to
4bb1730
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi)
pkg/util/admission/granter.go
line 471 at r1 (raw file):
updateExhaustedTokenDurationLocked(tokenCount, avail[w], sg.coordMu.availableIOTokens[w], settingAvailableTokens, &sg.exhaustedStart[w], sg.ioTokensExhaustedDurationMetric[w]) }
This looping and separation of the updateExhausterTokenDurationLocked with all these params is hard to follow. I think the following alternative is simpler:
func (sg *kvStoreTokenGranter) subtractTokensLocked(
count int64, elasticCount int64, settingAvailableTokens bool,
) {
sg.subtractTokensLockedForWorkClass(admissionpb.RegularWorkClass, count, settingAvailableTokens)
sg.subtractTokensLockedForWorkClass(admissionpb.ElasticWorkClass, elasticCount, settingAvailableTokens
if !settingAvailableTokens {
if count > 0 {
sg.tokensTakenMetric.Inc(count)
} else {
sg.tokensReturnedMetric.Inc(-count)
}
}
}
func (sg *kvStoreTokenGranter) subtractTokensLockedForWorkClass(wc admissionpb.WorkClass, count int64, settingAvailableTokens bool) {
sg.coordMu.availableIOTokens[wc] -= count
sg.availableTokensMetric[wc].Update(sg.coordMu.availableIOTokens[wc])
// Inline the code from updateExhaustedTokenDurationLocked.
...
}
4bb1730
to
375ad9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR, PTAL.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)
pkg/util/admission/granter.go
line 471 at r1 (raw file):
Previously, sumeerbhola wrote…
This looping and separation of the updateExhausterTokenDurationLocked with all these params is hard to follow. I think the following alternative is simpler:
func (sg *kvStoreTokenGranter) subtractTokensLocked( count int64, elasticCount int64, settingAvailableTokens bool, ) { sg.subtractTokensLockedForWorkClass(admissionpb.RegularWorkClass, count, settingAvailableTokens) sg.subtractTokensLockedForWorkClass(admissionpb.ElasticWorkClass, elasticCount, settingAvailableTokens if !settingAvailableTokens { if count > 0 { sg.tokensTakenMetric.Inc(count) } else { sg.tokensReturnedMetric.Inc(-count) } } } func (sg *kvStoreTokenGranter) subtractTokensLockedForWorkClass(wc admissionpb.WorkClass, count int64, settingAvailableTokens bool) { sg.coordMu.availableIOTokens[wc] -= count sg.availableTokensMetric[wc].Update(sg.coordMu.availableIOTokens[wc]) // Inline the code from updateExhaustedTokenDurationLocked. ... }
Done. Thanks for the suggestion, code looks much cleaner (and more aligned with what i was looking to do here).
This patch adds a new metric `elastic_io_tokens_exhausted_duration.kv`. This is similar to the existing `admission.granter.io_tokens_exhausted_duration.kv`, but for elastic traffic. The patch also does some code refactoring to make it easier to use both regular and elastic equivalent metrics. Informs cockroachdb#121574. Release note: None
375ad9b
to
708c361
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aadityasondhi)
bors r=sumeerbhola |
This patch adds the metric `elastic_io_tokens_exhausted_duration.kv` introduced in cockroachdb#124078. Informs cockroachdb#121572. Release note (ui change): The `Admission IO Tokens Exhausted` chart now separates elastic and regular io work.
This patch adds the metric `elastic_io_tokens_exhausted_duration.kv` introduced in cockroachdb#124078. Informs cockroachdb#121572. Release note (ui change): The `Admission IO Tokens Exhausted` chart now separates elastic and regular io work.
This patch adds the metric `elastic_io_tokens_exhausted_duration.kv` introduced in cockroachdb#124078. Informs cockroachdb#121572. Release note (ui change): The `Admission IO Tokens Exhausted` chart now separates elastic and regular io work.
This patch adds the metric `elastic_io_tokens_exhausted_duration.kv` introduced in cockroachdb#124078. Informs cockroachdb#121572. Release note (ui change): The `Admission IO Tokens Exhausted` chart now separates elastic and regular io work.
123522: dbconsole: overload page improvements r=sumeerbhola a=aadityasondhi This PR contains a series of improvements to the overload page of the DB console as part of #121574. It is separated into multiple commits for ease of review. ____ dbconsole: remove non useful charts on the overload page In investigations, we have found that the following charts are not useful and frequently cause confusion: - Admission work rate - Admission Delay rate - Requests Waiting For Flow Tokens Informs #121572 Release note (ui change): This patch removes "Admission Delay Rate", "Admission Work Rate", and "Requests Waiting For Flow Tokens". These charts often cause confusion and are not useful for general overload investigations. ___ dbconsole: reorder overload page metrics for better readability This patch reorders the existing metrics in a more usable order: 1. Metrics to help determine which resource is constrained (IO, CPU) 2. Metrics to narrow down which AC queues are seeing requests waiting 3. More advanced metrics about the system health (goroutine scheduler, L0 sublevels, etc.) Informs #121572. Release note (ui change): Reordering of metrics on the overload page to help categorizing them better. They are roughly in the following order: 1. Metrics to help determine which resource is constrained (IO, CPU) 2. Metrics to narrow down which AC queues are seeing requests waiting 3. More advanced metrics about the system health (goroutine scheduler, L0 sublevels, etc.) ___ dbconsole: include better names and descriptions for overload page This patch improves the metric descriptions for the metrics on the overload page. Fixes #120853. Release note (ui change): The overload page now includes descriptions for all metrics. ___ dbconsole: additional higher granularity metrics for overload This patch adds additional metrics to the overload page that allow for more granular look at the system: - cr.store.storage.l0-sublevels - cr.node.go.scheduler_latency-p99.9 Informs #121572. Release note (ui change): Two additional metrics on the overload page for better visibility into overloaded resources: - cr.store.storage.l0-sublevels - cr.node.go.scheduler_latency-p99.9 ___ dbconsole: split Admission Queue graphs to avoid overcrowding Informs #121572. Release note (ui change): There are now 4 graphs for Admission Queue Delay: 1. Foreground (regular) CPU work 2. Store (IO) work 3. Background (elastic) CPU work 4. Replication Admission Control, store overload on replicas ___ dbconsole: add elastic store metric to the overload page This patch uses the new sperated `elastic-stores` metrics for queing delay from #123890. Informs #121572. Release note (ui change): The `Admission Queueing Delay – Store` chart now separates elastic (background) work from the regular foreground work. ___ dbconsole: add elastic io token exhausted duration to overload page This patch adds the metric `elastic_io_tokens_exhausted_duration.kv` introduced in #124078. Informs #121572. Release note (ui change): The `Admission IO Tokens Exhausted` chart now separates elastic and regular io work. 124493: packer: only try emulating via Docker on x86 r=rail a=rickystewart Epic: none Release note: None Co-authored-by: Aaditya Sondhi <20070511+aadityasondhi@users.noreply.github.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
This patch adds a new metric
elastic_io_tokens_exhausted_duration.kv
. This is similar to the existingadmission.granter.io_tokens_exhausted_duration.kv
, but for elastic traffic.The patch also does some code refactoring to make it easier to use both regular and elastic equivalent metrics.
Informs #121574.
Release note: None