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

feat: mixin, allow overriding of some labels by parameterizing mixin recording/alert rules #11495

Merged
merged 23 commits into from
Oct 2, 2024

Conversation

alex5517
Copy link
Contributor

@alex5517 alex5517 commented Dec 15, 2023

What this PR does / why we need it:
Allows for more dynamic generation of recording/alert rules.

Which issue(s) this PR fixes:
Fixes N/A

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@alex5517 alex5517 requested a review from a team as a code owner December 15, 2023 07:25
@alex5517 alex5517 changed the title Feat/parameterize mixin mixin: parameterize mixin Dec 15, 2023
@alex5517 alex5517 changed the title mixin: parameterize mixin mixin: parameterize mixin recording/alert rules Dec 15, 2023
Copy link
Contributor

github-actions bot commented Dec 15, 2023

Trivy scan found the following vulnerabilities:

  • HIGH, Target: docker.io/grafana/loki:main-e93243f (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.3-r0. Fixed in v3.1.4-r0
  • HIGH, Target: docker.io/grafana/loki:main-e93243f (alpine 3.18.4), Type: alpine openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.3-r0. Fixed in v3.1.4-r0
    \nTo see more details on these vulnerabilities, and how/where to fix them, please run docker build -t grafana/loki:main-e93243f -f cmd/loki/Dockerfile .
    trivy i grafana/loki:main-e93243f on your branch. If these were not introduced by your PR, please considering fixing them in via a subsequent PR. Thanks!

Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

I think making the label selectors more modular is a good idea

production/loki-mixin/config.libsonnet Show resolved Hide resolved
production/loki-mixin/config.libsonnet Show resolved Hide resolved
production/loki-mixin/mixin-ssd.libsonnet Show resolved Hide resolved
@cstyan
Copy link
Contributor

cstyan commented Feb 22, 2024

@alex5517 looking good! I think we're more or less good to go. Unfortunately we need to merge/rebase on master to pull in changes for CI.

@cstyan cstyan changed the title mixin: parameterize mixin recording/alert rules feat: mixin, allow overriding of some labels by parameterizing mixin recording/alert rules Feb 23, 2024
@cstyan
Copy link
Contributor

cstyan commented Feb 23, 2024

@alex5517 sorry, one last change. Looks like the linter doesn't like the newline after the imports, or maybe there's a whitespace tab on that line?

@alex5517
Copy link
Contributor Author

@cstyan

Found the culprit :)

@cstyan
Copy link
Contributor

cstyan commented Feb 26, 2024

We also need to update the compiled mixins that are used for the helm chart Please build mixin by running 'make loki-mixin'

@alex5517
Copy link
Contributor Author

@cstyan,

Done :)

@cstyan
Copy link
Contributor

cstyan commented Apr 16, 2024

Sorry @alex5517 because I missed your earlier updates it looks like we need to pull in main and update the compiled dashboards again. I can get this merged this week assuming we can get all the CI checks green 👍

@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 18, 2024
@alex5517
Copy link
Contributor Author

@cstyan, i have merged changes from main, and re-run make loki-mixin

@cstyan
Copy link
Contributor

cstyan commented Apr 18, 2024

@alex5517 sorry, the mixin check is still failing because it thinks there should be additional changes to the compiled mixin. It might be due to jsonnet versions? The check uses jsonnet v0.20 and doesn't use our build image (Run make BUILD_IN_CONTAINER=false loki-mixin-check). I still need to update our build image to use that version of jsonnet.

@pull-request-size pull-request-size bot added size/M and removed size/L labels May 29, 2024
@alex5517
Copy link
Contributor Author

@cstyan,

Sorry for the delay, i synced the branch and ran the commands you asked.

Comment on lines +21 to +22
job_labels: [$._config.per_cluster_label, $._config.per_namespace_label, $._config.per_job_label],
cluster_labels: [$._config.per_cluster_label, $._config.per_namespace_label],
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make these names more explicit? even if it's long, I'd prefer to know all labels that are present from the name OR have to use each per_x_label for readability sake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cstyan,

I can change it, but would it not be better if it matches what mimir-mixin does, så that it easier for one who has used mimir-mixin to also use loki-mixin?

Copy link
Contributor

@cstyan cstyan 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 your patience @alex5517 , just one last comment I think

Signed-off-by: Alexander Soelberg Heidarsson <89837986+alex5517@users.noreply.github.com>
Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

cool, thanks @alex5517

@cstyan cstyan merged commit f1425b6 into grafana:main Oct 2, 2024
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants