feat: make agent stats' cardinality configurable #12535
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
…ebug works 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>
Signed-off-by: Danny Kopping <danny@coder.com>
mtojek
reviewed
Mar 12, 2024
Comment on lines
14
to
18
| "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.
Hhmm, I ran make 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.
goimports only ensures that imports are grouped, but doesn't handle merging multiple import groups from the same source. I recall that https://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.
Oh nice. I just ran it and it... changes a lot.
I'll raise a separate PR.
codersdk/deployment.go
Outdated
Comment on lines
18
to
22
| "github.com/coder/coder/v2/coderd/agentmetrics" | ||
|
|
||
| "github.com/coreos/go-oidc/v3/oidc" | ||
|
|
||
| "github.com/coder/coder/v2/buildinfo" |
| func filterInvalidLabels(labels []string) []string { | ||
| var out []string | ||
|
|
||
| for _, label := range labels { |
There was a problem hiding this comment.
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.
Should we add an internal_test.go to verify this function?
johnstcn
approved these changes
Mar 12, 2024
| "golang.org/x/xerrors" | ||
|
|
||
| "github.com/coder/coder/v2/coderd/agentmetrics" |
…/coder into dk/configurable-cardinality
Signed-off-by: Danny Kopping <danny@coder.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part 2 of #12468
We initially had to revert 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 in 3c1d016. 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 reviewing 3c1d016 should suffice.