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.

Sign up for GitHub

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

fix: incorrect pod matcher for compactor in mixin when using ssd mode #12846

Merged
merged 5 commits into from
Jul 17, 2024
Merged

fix: incorrect pod matcher for compactor in mixin when using ssd mode #12846

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2afdac
fix: incorrect pod matcher for compactor in mixin
QuentinBisson May 1, 2024
666e4db
Merge branch 'main' into fix-missing-compactor-matcher-in-mixin
QuentinBisson May 13, 2024
2f5ac23
Merge branch 'main' into fix-missing-compactor-matcher-in-mixin
QuentinBisson May 22, 2024
8f6d880
Merge branch 'main' into fix-missing-compactor-matcher-in-mixin
QuentinBisson Jul 8, 2024
d1591c8
simplify-compactor-matcher
QuentinBisson Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion production/loki-mixin-compiled-ssd/dashboards/loki-deletion.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@
"span": 4,
"targets": [
{
"expr": "go_memstats_heap_inuse_bytes{cluster=~\"$cluster\", namespace=~\"$namespace\", container=\"compactor\"} / 1024 / 1024 ",
"expr": "go_memstats_heap_inuse_bytes{cluster=~\"$cluster\", namespace=~\"$namespace\", container=\"loki\", pod=~\"(loki.*|enterprise-logs)-backend.*\"} / 1024 / 1024 ",
"format": "time_series",
"legendFormat": " {{pod}} ",
"legendLink": null
Expand Down
5 changes: 3 additions & 2 deletions production/loki-mixin/dashboards/loki-deletion.libsonnet
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ local utils = import 'mixin-utils/utils.libsonnet';
(import 'dashboard-utils.libsonnet') {
local compactor_matcher = if $._config.meta_monitoring.enabled
then 'pod=~"(compactor|%s-backend.*|loki-single-binary)"' % $._config.ssd.pod_prefix_matcher
else if $._config.ssd.enabled then 'container="loki", pod=~"%s-backend.*"' % $._config.ssd.pod_prefix_matcher else 'container="compactor"',
else if $._config.ssd.enabled then 'container="loki", pod=~"%s-backend.*"' % $._config.ssd.pod_prefix_matcher
else 'container="compactor"',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could have been okay without this change? and just doing the container="compactor" -> compactor_matcher change below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh definitely, I just thought it was easier to read. If you prefer I will change this tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

I think then 'pod=~"(compactor|%s-backend.*|loki-single-binary)"' % $._config.ssd.pod_prefix_matcher should cover the case where we were previously used container="compactor" right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should yes but I'm not familiar with loki in distributed mode.

Then we could change

local compactor_matcher = if $._config.meta_monitoring.enabled
  then 'pod=~"(compactor|%s-backend.*|loki-single-binary)"' % $._config.ssd.pod_prefix_matcher
  else if $._config.ssd.enabled then 'container="loki", pod=~"%s-backend.*"' % $._config.ssd.pod_prefix_matcher
  else 'container="compactor"'

to just:

local compactor_matcher = 'pod=~"(compactor|%s-backend.*|loki-single-binary)"' % $._config.ssd.pod_prefix_matcher

right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's mostly right, but it looks like it wouldn't work for %s-backend % $._config.ssd.pod_prefix_matcher because AFAICT the default value for that variable is (loki.*|enterprise-logs)

@MichelHollands can you confirm whether we really need separate selectors for when meta monitoring is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what would you like me to do here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichelHollands can you confirm what the matcher should be when meta montioring is enabled? I still think @QuentinBisson suggestion of local compactor_matcher = 'pod=~"(compactor|%s-backend.*|loki-single-binary)"' % $._config.ssd.pod_prefix_matcher covers all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @MichelHollands would you have time to check this one? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@QuentinBisson lets move forward with the simplified matcher you've suggested and open follow ups with fixes if anyone reports any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's pushed :)

grafanaDashboards+::
{
'loki-deletion.json':
Expand Down Expand Up @@ -50,7 +51,7 @@ local utils = import 'mixin-utils/utils.libsonnet';
)
.addPanel(
$.newQueryPanel('Compactor memory usage (MiB)') +
g.queryPanel('go_memstats_heap_inuse_bytes{%s, container="compactor"} / 1024 / 1024 ' % $.namespaceMatcher(), ' {{pod}} '),
g.queryPanel('go_memstats_heap_inuse_bytes{%s, %s} / 1024 / 1024 ' % [$.namespaceMatcher(), compactor_matcher], ' {{pod}} '),
)
.addPanel(
$.newQueryPanel('Compaction run duration (seconds)') +
Expand Down
Loading