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

Alloy-Mixin: allow k8s cluster and alloy cluster disable, add logs dashboard #808

Merged
merged 6 commits into from May 24, 2024

Conversation

gaantunes
Copy link
Contributor

@gaantunes gaantunes commented May 8, 2024

PR Description

Introducing the following changes:

  • including a config file to the mixin, which allows you to:
    • disable dashboards, panels and alerts that are targeted on monitoring alloy instances being run in cluster mode.
    • disable the inclusion of k8s cluster targeted labels on dashboards and alerts.
    • change the dashboard tags
  • including a logs dashboard
  • including a job label everywhere as a grouper when cluster and namespace are disabled, to avoid metric conflicts - like up metric

Which issue(s) this PR fixes

Notes to the Reviewer

Let me know if this needs changelog updates or if there are any docs.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@gaantunes gaantunes marked this pull request as draft May 8, 2024 22:05
@gaantunes gaantunes marked this pull request as ready for review May 20, 2024 21:29
@gaantunes gaantunes requested a review from rfratto May 20, 2024 21:32
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thanks! This is a bit hard to review for concrete differences at the moment given how many things changed, but I left a few comments to help reduce the number of differences so I can give a more thoughtful second pass.

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

operations/alloy-mixin/config.libsonnet Outdated Show resolved Hide resolved
operations/alloy-mixin/alerts.libsonnet Outdated Show resolved Hide resolved
operations/alloy-mixin/alerts/clustering.libsonnet Outdated Show resolved Hide resolved

local labels = if $._config.enableK8sCluster then ['cluster', 'namespace', 'job', 'instance', 'level'] else ['job', 'instance', 'level'],

grafanaDashboards+:
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to follow the pattern that other dashboards do; this is the only one with a grafanaDashboards top-level key. Can we change this to be consistent with the other dashboards?

That would also mean moving the logic for whether to enable the dashboard outside of the definition of the dashboard itself, which would be more consistent with how the clustering dashboards are made optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is because the library I am using expects a grafanaDashboards variable.
I also don't like how it looks different, but since this is a totally different library which is synced with the latest version of grafonnet, I believe it is an acceptable trade off.

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

No other comments from me, nice work making this configurable!

@rfratto rfratto merged commit a30cc8a into main May 24, 2024
18 checks passed
@rfratto rfratto deleted the gaantunes/mixin-improvements branch May 24, 2024 16:35
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.

None yet

2 participants