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(promtail-mixin): improve standalone usage #8534

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Zaunei
Copy link

@Zaunei Zaunei commented Feb 15, 2023

What this PR does / why we need it:

This PR improves the usage of the promtail-mixin in setups where no Loki is installed and writes metrics in the same tenant.

With the default Helm installation of promtail, the job label (job=~"promtail-metrics") does not match the currently hard-coded label(job=~"$namespace/promtail"), so the latency graphs will not work. Since the selectors will always differ slightly from setup to setup, I made them configurable.

Added:

  • Extend the Makefile to compile promtail-mixins
  • Compiled promtail-mixins

Changed:

  • Make Dashboard name and label selectors configurable
  • Adopt loki-mixin file structure/ Jsonnet stlye

Which issue(s) this PR fixes:

No issue created, as I consider this to be a non-breaking fix and improvement.

Special notes for your reviewer:

In order not to cause any braking changes I have taken over the values used before.
Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@Zaunei Zaunei requested a review from a team as a code owner February 15, 2023 14:17
@CLAassistant
Copy link

CLAassistant commented Feb 15, 2023

CLA assistant check
All committers have signed the CLA.

@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from e575c87 to d58c7a4 Compare February 22, 2023 14:25
@Zaunei Zaunei marked this pull request as draft February 22, 2023 20:55
@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from d58c7a4 to ecf6f90 Compare February 22, 2023 21:32
@Zaunei Zaunei marked this pull request as ready for review February 22, 2023 21:47
@Zaunei
Copy link
Author

Zaunei commented Feb 22, 2023

Found another bug. I could not overwrite the config from the outside, so I fixed the config references and oriented myself to the loki-mixin dashboard generation, also to increase the conciseness here.
Now I can overwrite the config while importing the mixin.

@cstyan
Copy link
Contributor

cstyan commented Nov 7, 2023

@Zaunei this looks nice 👍 if the fixes here are still relevant to you please update to fix the merge conflicts, and I'll see if we can test deploying this quickly ourselves internally just to confirm nothing's broken

@Zaunei
Copy link
Author

Zaunei commented Nov 17, 2023

Sure, I will update this PR. In the meantime, ne part of this PR got already upstreamed with #9684.

@Zaunei Zaunei changed the title promtail-mixin: Improve and fix standalone usage promtail-mixin: Improve standalone usage Nov 17, 2023
@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from ecf6f90 to 70e9ee6 Compare November 17, 2023 14:21
Copy link
Contributor

Trivy scan found the following vulnerabilities:

@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from 70e9ee6 to 4fcdbd7 Compare November 17, 2023 14:27
Zaunei added a commit to Zaunei/loki that referenced this pull request Nov 17, 2023
@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from 4fcdbd7 to bb9b259 Compare November 17, 2023 14:31
@Zaunei
Copy link
Author

Zaunei commented Nov 17, 2023

@cstyan PR is now uptodate. Happy to hear feedback :)

@cstyan
Copy link
Contributor

cstyan commented Dec 4, 2023

@trevorwhitney does the compiled mixin here make sense?

@cstyan
Copy link
Contributor

cstyan commented Dec 21, 2023

@Zaunei sorry for the delay in response again, Trevor is the one who's done the compiled mixins in the past but unfortunately he's out for the rest of the year AFAIK.

@Zaunei
Copy link
Author

Zaunei commented Jan 22, 2024

@cstyan @trevorwhitney Is there anything that needs to be improved for this to be merged?

Zaunei added a commit to Zaunei/loki that referenced this pull request Feb 12, 2024
@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from bb9b259 to 93adc71 Compare February 12, 2024 16:51
@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from 93adc71 to 0169618 Compare April 4, 2024 07:59
Zaunei added a commit to Zaunei/loki that referenced this pull request Apr 4, 2024
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

I don't think we need to pre-compile it here but I'm fine either way

@Zaunei Zaunei changed the title promtail-mixin: Improve standalone usage feat(promtail-mixin): improve standalone usage May 24, 2024
@Zaunei Zaunei force-pushed the promtail-mixin-standalone branch from 0169618 to d163387 Compare May 24, 2024 10:21
@Zaunei
Copy link
Author

Zaunei commented May 24, 2024

Thanks for your approval @trevorwhitney!
Since there were merge conflicts and small things were changed by #12527, so I rebased the branch again. I also changed the commit messages directly to conventional commits and dropped the commit for the Changelog.md update. Other than that, I haven't made any changes here and the PR should now be ready again.

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.

4 participants