Skip to content
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

Keep searching for job/instance if multiple job/instance matchers present #206

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

v-zhuravlev
Copy link
Contributor

@v-zhuravlev v-zhuravlev commented Nov 5, 2024

This improvement would keep searching for job/instance matchers in PromQL query instead of failing on the first match.

In this case [target-job-rule] check would not fail if multiple job/instance matchers are present and expected value (i.e job=~"$job") is within the second, not the first match.

This should fix it when job is present in both groupLabels and filteringSelector.

For example:

$instance"}': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'Disk C: size', target idx '0' invalid PromQL query 'windows_logical_disk_size_bytes{volume="C:", job=~".*windows.*",job=~"$job",instance=~"$instance"}': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'CPU usage', target idx '0' invalid PromQL query '100 - (avg without (mode,core) (rate(windows_cpu_time_total{mode="idle", job=~".*windows.*",job=~"$job",instance=~"$instance"}[$__rate_interval])*100))': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'CPU usage', target idx '0' invalid PromQL query '100 - (avg without (mode,core) (rate(windows_cpu_time_total{mode="idle", job=~".*windows.*",job=~"$job",instance=~"$instance"}[$__rate_interval])*100))': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'Memory usage', target idx '0' invalid PromQL query '100 - windows_os_physical_memory_free_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"} / windows_cs_physical_memory_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"} * 100': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'Memory usage', target idx '0' invalid PromQL query '100 - windows_os_physical_memory_free_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"} / windows_cs_physical_memory_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"} * 100': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'Memory usage', target idx '0' invalid PromQL query 'windows_cs_physical_memory_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"} - windows_os_physical_memory_free_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"}': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'Memory usage', target idx '0' invalid PromQL query 'windows_cs_physical_memory_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"} - windows_os_physical_memory_free_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"}': job selector is .*windows.*, not $job
[target-job-rule] 'Windows overview': Dashboard 'Windows overview', panel 'Memory usage', target idx '1' invalid PromQL query 'windows_cs_physical_memory_bytes{job=~".*windows.*",job=~"$job",instance=~"$instance"}': job selector is .*windows.*, not $job

Copy link
Member

@Dasomeone Dasomeone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, will test the other PR in the morning :)

@v-zhuravlev
Copy link
Contributor Author

@rgeyer can we merge this?

@rgeyer
Copy link
Collaborator

rgeyer commented Nov 6, 2024

@rgeyer can we merge this?

@Dasomeone and/or @v-zhuravlev, can I get a review of #208 before I merge this? It is a superset of #181 that's been in the works for a long time, so an in-depth review isn't exactly necessary (tho appreciated). It is a bit of a monster and we need to get it merged before much more changes in the repo.

LMK if you have any questions! :)

Copy link
Collaborator

@rgeyer rgeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, ignore me, this one is super straightforward and won't cause any conflicts. I'm just hyper sensitive to changes against main right now until we get that other PR settled.

@rgeyer rgeyer merged commit 1e79993 into main Nov 6, 2024
7 checks passed
@rgeyer rgeyer deleted the vzhuravlev/job-multiple branch November 6, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants