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

Jump to bottom

fix(helm): Disable service monitor for nginx service #12746

Merged
merged 4 commits into from
Feb 5, 2025
Merged

fix(helm): Disable service monitor for nginx service #12746

merged 4 commits into from
Feb 5, 2025

Conversation

adinhodovic
Copy link
Contributor

@adinhodovic adinhodovic commented Apr 23, 2024 •
edited
Loading

What this PR does / why we need it:
The service monitor that targets all Loki pods targets nginx as well. Nginx isn't configured for prometheus metrics and the endpoint is 404, which causes alerts that the target is down.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
  • 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

@adinhodovic adinhodovic requested a review from a team as a code owner April 23, 2024 10:21
@CLAassistant
Copy link

CLAassistant commented Apr 23, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@adinhodovic adinhodovic force-pushed the disable-service-monitor-for-nginx branch from 4eef37d to d29818b Compare April 23, 2024 10:21
@JStickler
Copy link
Contributor

Please update the other files listed in the checklist. 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

@adinhodovic adinhodovic force-pushed the disable-service-monitor-for-nginx branch from d29818b to 1c2b5a1 Compare April 23, 2024 16:15
@adinhodovic
Copy link
Contributor Author

Please update the other files listed in the checklist. 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

Makes sense, fixed.

@adinhodovic adinhodovic force-pushed the disable-service-monitor-for-nginx branch from e9fbfc8 to 157d683 Compare May 14, 2024 09:13
@adinhodovic
Copy link
Contributor Author

Please update the other files listed in the checklist. 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

Anything else needed to move this?

@pull-request-size pull-request-size bot added size/XS and removed size/S labels Jul 2, 2024
@adinhodovic
Copy link
Contributor Author

@JStickler Can you merge this small PR?

@adinhodovic adinhodovic mentioned this pull request Jul 2, 2024
Closed
7 tasks
@JStickler
Copy link
Contributor

@adinhodovic I'm the technical writer on the team, so I avoid merging code as I'm not a developer.

@adinhodovic
Copy link
Contributor Author

adinhodovic commented Jul 2, 2024 •
edited
Loading

@adinhodovic I'm the technical writer on the team, so I avoid merging code as I'm not a developer.

Ah, sorry about the ping!

@adinhodovic
Copy link
Contributor Author

Is this fine @vlad-diachenko?

@adinhodovic adinhodovic force-pushed the disable-service-monitor-for-nginx branch 2 times, most recently from 668bea6 to 0febbe0 Compare February 3, 2025 10:17
vlad-diachenko
vlad-diachenko reviewed Feb 4, 2025
View reviewed changes
Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

huge thanks for the contribution, @adinhodovic . please address my comment and we can merge it

@adinhodovic adinhodovic force-pushed the disable-service-monitor-for-nginx branch from 0febbe0 to 82bb4be Compare February 4, 2025 10:51
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Feb 4, 2025
@adinhodovic
Copy link
Contributor Author

huge thanks for the contribution, @adinhodovic . please address my comment and we can merge it

fixed @vlad-diachenko!

vlad-diachenko
vlad-diachenko approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

lgtm 🥇 thanks for the contribution ❤️

@vlad-diachenko vlad-diachenko enabled auto-merge (squash) February 4, 2025 15:16
@vlad-diachenko vlad-diachenko merged commit 0c38b94 into grafana:main Feb 5, 2025
66 checks passed
@weikinhuang
Copy link

hi, this pr is now causing a yaml validation error

stdin - Service release-name-loki-gateway failed validation: error unmarshalling resource: error converting YAML to JSON: yaml: unmarshal errors:
  line 14: key "prometheus.io/service-monitor" already set in map
Summary: 30 resources found parsing stdin - Valid: 29, Invalid: 0, Errors: 1, Skipped: 0

@adinhodovic
Copy link
Contributor Author

adinhodovic commented Feb 5, 2025 •
edited
Loading

hi, this pr is now causing a yaml validation error

stdin - Service release-name-loki-gateway failed validation: error unmarshalling resource: error converting YAML to JSON: yaml: unmarshal errors:
  line 14: key "prometheus.io/service-monitor" already set in map
Summary: 30 resources found parsing stdin - Valid: 29, Invalid: 0, Errors: 1, Skipped: 0

Are you adding that key additionally in the values file while it is already being set by default?

@weikinhuang
Copy link

Ah, I am, sorry for the noise. I applied the label manually in my helm values last year b/c I ran into this issue. The error was from my automated ci process.

@adinhodovic
Copy link
Contributor Author

Ah, I am, sorry for the noise. I applied the label manually in my helm values last year b/c I ran into this issue. The error was from my automated ci process.

No worries, then it's doing what it's supposed to!

@asherf
Copy link

asherf commented Feb 5, 2025

FWIW - this is not ideal... having nginx being scraped (using the patch suggested here: #9522 (comment) is still useful, in to be able to alert when getting HTTP 50x or HTTP 40x from the gateway..
at the very least... allow people the option to not have this label... (w/o needing an enterprise version).

@LaurinStreng LaurinStreng mentioned this pull request Feb 6, 2025
Open
salvacorts pushed a commit that referenced this pull request Feb 12, 2025
@adinhodovic @vlad-diachenko @salvacorts
fix(helm): Disable service monitor for nginx service (#12746)
0ba078e 
Co-authored-by: Vladyslav Diachenko <[email protected]>
@loki-gh-app loki-gh-app bot mentioned this pull request Feb 17, 2025
Open
@loki-gh-app loki-gh-app bot mentioned this pull request Feb 24, 2025
Open
@loki-gh-app loki-gh-app bot mentioned this pull request Mar 5, 2025
Open
@Routhinator
Copy link

As mentioned in #13201 - this fix does not work.

This was referenced Mar 20, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlad-diachenko vlad-diachenko vlad-diachenko approved these changes

Assignees
No one assigned
Labels
area/helm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@adinhodovic @CLAassistant @JStickler @weikinhuang @asherf @Routhinator @vlad-diachenko