- Notifications
You must be signed in to change notification settings - Fork928
feat: make agent stats' cardinality configurable#12535
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#12535
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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>
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>
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>
| "github.com/coder/coder/v2/coderd/agentmetrics" | ||
| "github.com/coder/coder/v2/coderd/batchstats" | ||
| "github.com/coder/coder/v2/coderd/database/dbtestutil" | ||
| "github.com/coder/coder/v2/coderd/database/dbtime" |
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.
nit: sorting?
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.
Hhmm, I ranmake fmt and it didn't correct it so I think we have some things to fix in our CI. Will manually sort though 👍
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.
goimports only ensures that imports are grouped, but doesn't handle merging multiple import groups from the same source. I recall thathttps://github.com/indeedeng/go-groups was built in anger to address this, but it has not been updated in a while.
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.
Oh nice. I just ran it and it... changesa lot.
I'll raise a separate PR.
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.
It isextremely opinionated
codersdk/deployment.go Outdated
| "github.com/coder/coder/v2/coderd/agentmetrics" | ||
| "github.com/coreos/go-oidc/v3/oidc" | ||
| "github.com/coder/coder/v2/buildinfo" |
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.
nit: here too
| func filterInvalidLabels(labels []string) []string { | ||
| var out []string | ||
| for _, label := range labels { |
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.
maybe optimize it this way?
validLabels := make([]string, 0, len(labels))for _, label := range labels {if label != agentmetrics.LabelTemplateName {validLabels = append(validLabels, label)}}| // filterInvalidLabels handles a slightly messy situation whereby `prometheus-aggregate-agent-stats-by` can control on | ||
| // which labels agent stats are aggregated, but for these specific metrics in this file there is no `template` label value, | ||
| // and therefore we have to exclude it from the list of acceptable labels. | ||
| func filterInvalidLabels(labels []string) []string { |
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.
Should we add aninternal_test.go to verify this function?
johnstcn left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
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.
Only a couple of nits from me. Tested locally with a small scaletest.
Uh oh!
There was an error while loading.Please reload this page.
| "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.
nit: sorting
…/coder into dk/configurable-cardinality
Signed-off-by: Danny Kopping <danny@coder.com>
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.
👍
Uh oh!
There was an error while loading.Please reload this page.
Part 2 of#12468
We initially had torevert the above PR because of a panic from Prometheus.
This was caused by the 4 default labels being passed in to
AgentStats(), however these metrics do not contain a value for thetemplate_namelabel. This was masked in a test by passing innilfor the aggregation labels which defaulted them to the 3 correct ones, but I subsequently changed the default value fromnilin the actual code to all 4 labels for various reasons.I implemented the fix in3c1d016. I can't think of a cleaner approach than this; if you come up with a better approach, please share!
Note to reviewers: the code is otherwise identical to#12468, so simply reviewing3c1d016 should suffice.