database_observability: add auto-enable setup consumers to query_sample #3922
Conversation
cristiangreco
force-pushed
the
cristian/dbo11y-query-sample-autoenable-cpu-consumer
branch
from
941c4f7 to
7219121
Compare
| DataSourceName alloytypes.Secret `alloy:"data_source_name,attr"` | ||
| CollectInterval time.Duration `alloy:"collect_interval,attr,optional"` | ||
| ForwardTo []loki.LogsReceiver `alloy:"forward_to,attr"` | ||
| EnableCollectors []string `alloy:"enable_collectors,attr,optional"` |
There was a problem hiding this comment.
I wonder, (not sure if it's possible), in the future when we have several different setup_consumers we want to configure in a fine-grained manner, will it be easier provide a list to enable/disable similar to EnableCollectors/DisableCollectors?
Might be overengineered, though, so maybe it's better to start with a simple boolean for now that is at the lowest level.
There was a problem hiding this comment.
Hmm something to think about, but agree to start simple for now as we don't know how widespread this functionality will be.
|
Should we distinguish because something that "collects" and something that will "write"? |
cristiangreco
force-pushed
the
cristian/dbo11y-query-sample-autoenable-cpu-consumer
branch
3 times, most recently
from
d8e5886 to
d5a6d9a
Compare
|
💻 Deploy preview available: |
cristiangreco
force-pushed
the
cristian/dbo11y-query-sample-autoenable-cpu-consumer
branch
from
d5a6d9a to
eec1c14
Compare
|
I wonder if we should create a separate collector that is responsible for maintaining all write operations based on enabled colletors array (taking into account defaults of course). Detaching it would allow a longer scrape period for these operations, instead of checking/writing every time we do a scrape in each collector. |
cristiangreco
force-pushed
the
cristian/dbo11y-query-sample-autoenable-cpu-consumer
branch
2 times, most recently
from
22b1cb9 to
221ac06
Compare
Introduce a check in the `query_sample` collector to automatically enable the `events_statements_cpu` consumer in the `performance_schema.setup_consumers` table. This is guarded by two config options: - `allow_update_performance_schema_settings`: to enable "updates" to performance_schema settings altogether. - `auto_enable_setup_consumers`: to start auto-enabling of the setting specific to the `query_sample` collector.
cristiangreco
force-pushed
the
cristian/dbo11y-query-sample-autoenable-cpu-consumer
branch
from
221ac06 to
623ef02
Compare
Yes I agree the the two things should not be necessarily tied together. I'd prefer to postpone that for a followup PR though if that's ok (so here we first verify this is working and proves valuable, then optimise it). |
| if c.autoEnableSetupConsumers { | ||
| if err := c.updateSetupConsumersSettings(ctx); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
As commented by @gaantunes we could decouple the update from the regular collector loop. I'm keen to postpone that work for a followup PR, here I'm more interested in the correctness of the approach.
cristiangreco
requested review from
matthewnolf,
clayton-cornell and
a team
as code owners
PR Description
Introduce a check in the
query_samplecollector to automatically enable theevents_statements_cpuconsumer in theperformance_schema.setup_consumerstable.This is guarded by two config options:
allow_update_performance_schema_settings: to enable "updates" to performance_schema settings altogether.auto_enable_setup_consumers: to start auto-enabling of the setting specific to thequery_samplecollector.Which issue(s) this PR fixes
n.a.
Notes to the Reviewer
PR Checklist