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

Loki Prometheus mixin: Fix incorrect selectors and simplify mixin usage #6189

Merged
merged 27 commits into from
Jun 28, 2022
Merged

Conversation

irizzant
Copy link
Contributor

@irizzant irizzant commented May 18, 2022

Signed-off-by: irizzant i.rizzante@gmail.com

What this PR does / why we need it:

This PR fixes some issues reported in #4838 with selectors and matchers.

It also adds the kube-prometheus mixin utils needed to automatically generate PrometheusRule objects and dashboards.

This way using the mixin become as easy as adding the following to the main.jsonnet:

{ // this is the main object in main.jsonnet
local addMixin = (import 'github.com/prometheus-operator/kube-prometheus/jsonnet/kube-prometheus/lib/mixin.libsonnet');
local lokiMixinRulesDashboards = addMixin({
  name: 'loki',
  mixin: (import 'github.com/grafana/loki/production/loki-mixin/mixin.libsonnet') + {
    _config+: {},  // mixin configuration object
  },

prometheusRules: lokiMixinRulesDashboards.prometheusRules {
        metadata+: k.meta.v1.objectMeta.withNamespace($.namespace),
      },
});

} + {
      // this adds the ConfigMap's with dashboards json
      local configMap = k.core.v1.configMap,

      [name]: configMap.new('loki-dashboard' + name) +
              configMap.withData({
                [name]: std.manifestJsonMinified(lokiMixinRulesDashboards.grafanaDashboards[name]),
              }) +
              configMap.metadata.withLabelsMixin({
                grafana_dashboard: '1',
              })

      for name in std.objectFields(lokiMixinRulesDashboards.grafanaDashboards)
    },

  }

Which issue(s) this PR fixes:
Fixes #4838

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

Signed-off-by: irizzant <i.rizzante@gmail.com>
@irizzant irizzant requested a review from a team as a code owner May 18, 2022 10:54
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 18, 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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@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%

1 similar comment
@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%

@irizzant irizzant changed the title Loki Prometheus mixin: fix incorrect selectors and matchers Loki Prometheus mixin: Fix incorrect selectors and matchers May 18, 2022
@irizzant irizzant changed the title Loki Prometheus mixin: Fix incorrect selectors and matchers Loki Prometheus mixin: Fix incorrect selectors and simplify mixin usage May 18, 2022
Signed-off-by: irizzant <i.rizzante@gmail.com>
@irizzant
Copy link
Contributor Author

@kavirajk can you please have a look at this too? I don't understand why CI is failing since I didn't change anything but jsonnet

@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%

@irizzant
Copy link
Contributor Author

irizzant commented May 18, 2022

@kavirajk

I don't understand why CI is failing since I didn't change anything but jsonnet

never mind, it looks like it automagically fixed itself!

@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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@irizzant
Copy link
Contributor Author

Hey @kavirajk can you have a look at this please?

@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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@irizzant
Copy link
Contributor Author

@kavirajk I resolved the discussion related to this PR , let me know if it's ok for you

@irizzant
Copy link
Contributor Author

irizzant commented Jun 3, 2022

@kavirajk can you please let me know if you need anything else to merge this PR ?

Thank you

@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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@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%

Signed-off-by: irizzant <i.rizzante@gmail.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 14, 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%

@pull-request-size pull-request-size bot added size/M and removed size/L labels Jun 14, 2022
@irizzant
Copy link
Contributor Author

Hi @trevorwhitney is it possible to get the PR merged? I had to solve some conflicts...

@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%

@irizzant
Copy link
Contributor Author

Hi @trevorwhitney @kavirajk , can you please have a look? I fixed the conflicts and it should be ready to be merged now

@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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Collaborator

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

@irizzant Thanks for your patience. LGTM, approach with service monitor.

@kavirajk
Copy link
Collaborator

@irizzant Looks like mixin-check is failing. Can you take care of it? I can merge after that.

@irizzant irizzant requested a review from kavirajk June 27, 2022 12:38
@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.3%
+            querier	0%
- querier/queryrange	-0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

@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%

@irizzant
Copy link
Contributor Author

@kavirajk @trevorwhitney , the checks are passing.

I don't know why they failed before, I just merged the main branch.

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.

loki-mixin: Complex configuration needed compared to other mixins
4 participants