Skip to content

Conversation

agologan
Copy link
Contributor

@agologan agologan commented Oct 27, 2022

What this PR does / why we need it:
If you're using prometheus operator and not Grafana Agent a metricsInstance shouldn't be auto-created.

Which issue(s) this PR fixes:
#7529

Special notes for your reviewer:
I would also propose moving the definition at the same level with serviceMonitor and adding .enabled i.e.:

  serviceMonitor:
    enabled: true
    ...
  metricsInstance:
    enabled: true
    annotations: {}
    labels: {}
    remoteWrite: null

This of course would be a breaking change.
L.E. Just noticed this other PR: #7525

Checklist

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

@agologan agologan requested a review from a team as a code owner October 27, 2022 13:03
@agologan agologan changed the title Helm: create metricsInstance only if available [Helm] Create metricsInstance only if available Oct 27, 2022
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

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'd like to see a combination of this and #7525. I like where the enabled flag was added in #7525 as I feel it's a bit less disruptive, but I also like you capabilities check. Would you mind refactoring once #7525 is in to add your check to that same conditional where the new enabled is checked?

@agologan agologan force-pushed the fix-metricsinstance-crd-unavailable branch from 425b90e to f76ee4e Compare November 1, 2022 21:10
@agologan
Copy link
Contributor Author

agologan commented Nov 1, 2022

@trevorwhitney updated.
While I could've just written this big and, used the same pattern present in other files to be a bit more consistent.

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

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.

lgtm, looks good, thanks!

@trevorwhitney trevorwhitney merged commit 726e460 into grafana:main Nov 8, 2022
Abuelodelanada pushed a commit to canonical/loki that referenced this pull request Dec 1, 2022
**What this PR does / why we need it**:
If you're using prometheus operator and not Grafana Agent a
metricsInstance shouldn't be auto-created.

**Which issue(s) this PR fixes**:
grafana#7529
mraboosk pushed a commit to mraboosk/loki that referenced this pull request Oct 7, 2024
**What this PR does / why we need it**:
If you're using prometheus operator and not Grafana Agent a
metricsInstance shouldn't be auto-created.

**Which issue(s) this PR fixes**:
grafana#7529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants