- Notifications
You must be signed in to change notification settings - Fork928
feat: make agent stats' cardinality configurable#12468
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat: make agent stats' cardinality configurable#12468
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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.
Noticed some minor nits, but in general look good, nice work!
"golang.org/x/xerrors" | ||
"github.com/coder/coder/v2/coderd/agentmetrics" |
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.
Unfortunately our formatter doesn't handle merging import groups and leaves things in a messy state (depending on what program injected them). 😔
If you notice these, please feel free to fix, but the standard is we try our best but sometimes these slip through, so don't worry too much.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
codersdk/deployment.go Outdated
return nil | ||
} | ||
acceptable := make(map[string]any, len(AcceptedMetricAggregationLabels)) |
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.
I think usingmap[string]bool
would be preferable/clearer here, also simplifies the map lookup later. Typically I'd use eitherbool
or if we're looking for space savings, I'd use the zero struct (map[string]struct{}
).
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
…ebug worksSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
5ba04c5
to3e569ff
CompareSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Danny Kopping <danny@coder.com>
…/coder into dk/configurable-cardinality
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
881cd6a
to9b16a3b
Compare…k/configurable-cardinality
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.
LGTM! I know tests aren't passing now but looks unrelated so I don't need to re-review, nice work!
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.
Nice!
I know tests aren't passing now but looks unrelated so I don't need to re-review, nice work!
Agree, feel free tot.Skip
them for now.
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Implements#12221
When stats from agents are collected, they are aggregated by 4 dimensions:
agent_name
,template_name
,workspace_name
, andusername
. This can result in some very high cardinality metrics being scraped by Prometheus in large environments.This PR adds the ability to tune which labels are include in this aggregation, therefore reducing the cardinality.
For example:
With hundreds of active workspaces, each having a unique name, the cardinality may be unacceptable. In this case the operator may choose to configure
CODER_PROMETHEUS_AGGREGATE_AGENT_STATS_BY=username
to rather aggregate by user, summing all the metrics' values and only producing a single metric series:Multiple labels can be provided, e.g.
CODER_PROMETHEUS_AGGREGATE_AGENT_STATS_BY=username,agent_name
The current behaviour remains the default; if no value is passed to
CODER_PROMETHEUS_AGGREGATE_AGENT_STATS_BY
there will be no change.Note to reviewer: I made a sweeping refactor to define the label names in a single location instead of duplicating them, which may make the PR larger than it seems.