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

Improve jsonnet parser for rules #211

Merged
merged 3 commits into from
Apr 18, 2022
Merged

Conversation

v-zhuravlev
Copy link
Contributor

Hi! Even though it says deprecated for using hidden fields, we still using it extensively for monitoring-mixins development, especially in grr watch mode.

It works great for 'dashboards only' mixins that way, but unfortunately, it stops working when rules are present, error is recevied:

2022-04-17 02:34:04.863636 I | 02:34:04 Error:  RUNTIME ERROR: Unexpected type string, expected number
        mixin.libsonnet:69:28-65
        mixin.libsonnet:75:17-31        thunk from <thunk from <function <fromMap>>>
        mixin.libsonnet:8:58-62 thunk from <thunk from <thunk from <object <anonymous>>>>
        mixin.libsonnet:8:43-76 thunk from <thunk from <object <anonymous>>>
        mixin.libsonnet:8:28-90 thunk from <object <anonymous>>
        mixin.libsonnet:8:13-103        object <anonymous>
        During manifestation

This is due grizzly.jsonnet expects rules to be wrapped in the namespace:

  prometheusAlerts+:: {
    grizzly_rules: { <--namespace
      groups: [{
        name: 'my_rules_group',
        rules: [
          $.alert,
        ],
      }],
    },
  },

while monitoring-mixins by convention have alerts defined without one:

  _config+:: {
  ...
  },
  grafanaDashboards+:: {
  ...
  },
  prometheusRules+:: {
  ...
  },
  prometheusAlerts+:: {
      groups: [{
        name: 'my_rules_group',
        rules: [
          $.alert,
        ],
      }],
  },

(some examples:
https://github.com/prometheus/node_exporter/blob/master/docs/node-mixin/alerts/alerts.libsonnet
https://github.com/prometheus/prometheus/blob/main/documentation/prometheus-mixin/alerts.libsonnet
)

So here is the suggested improvement for jsonnet rule parser that:

  • Parses rule groups even if they are not wrapped into namespace. This is extremely useful when working with monitoring-mixins. 'grizzly_rules' namespace is added
  • Loads all rule groups instead of the first one.

This fix not only allows loading of monitoring-mixins dashboards inside grafana without any grizzly specific changes to original mixin, but also can load monitoring-mixins alerts into cortex as well:

user@vzhuravl-mac node-mixin % ../../../grizzly/cmd/grr/grr  watch .  mixin.libsonnet 
INFO[0000] Watching for changes                         
INFO[0001] Changes detected. Applyingmixin.libsonnet    
INFO[0001] Applying 8 resources                         
Dashboard.node-rsrc-use updated
Dashboard.nodes-darwin updated
Dashboard.nodes updated
Dashboard.node-multicluster-rsrc-use updated
PrometheusRuleGroup.grizzly_rules.node-exporter.rules no differences
PrometheusRuleGroup.abc.node-exporter-small no differences
PrometheusRuleGroup.zzz.node-exporter-small2 no differences
PrometheusRuleGroup.zzz.node-exporter no differences

image

Copy link
Collaborator

@malcolmholmes malcolmholmes left a comment

Choose a reason for hiding this comment

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

Minor comments, looks otherwise done well, thanks!

Comment on lines 80 to 93
[
[
makeResource(
'PrometheusRuleGroup',
g.name,
spec={
rules: g.rules,
},
metadata={ namespace: ns }
)
for g in allNamespaced[ns].groups
]
for ns in std.objectFields(allNamespaced)
] else [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

if key in main is true, then doesn't this result in an array within an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Still, grizzly tolerated that :) Updated to use nested loop without extra array in the result.

docs/content/hidden-elements.md Outdated Show resolved Hide resolved
- Load all rule groups instead of the first one
- Parse rule groups when they are not wrapped into namespace. This is extremely useful when working with monitoring-mixins.
Co-authored-by: malcolmholmes <42545407+malcolmholmes@users.noreply.github.com>
Copy link
Collaborator

@malcolmholmes malcolmholmes left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for this.

@malcolmholmes malcolmholmes merged commit a02770b into master Apr 18, 2022
@v-zhuravlev v-zhuravlev deleted the vzhuravlev/rules_from_mixins branch April 18, 2022 16:30
@v-zhuravlev v-zhuravlev self-assigned this Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants