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

Remove unit from alerts #239

Merged
merged 6 commits into from
Mar 9, 2022
Merged

Remove unit from alerts #239

merged 6 commits into from
Mar 9, 2022

Conversation

dstathis
Copy link
Contributor

@dstathis dstathis commented Mar 4, 2022

No description provided.

@balbirthomas
Copy link
Contributor

@dstathis It would help in review and for posterity if your commit message included

  • A description of the bug
  • An explanation why this commit is the right fix for the problem.

rbarry82
rbarry82 previously approved these changes Mar 4, 2022
This commit adds unit tests to check the following

- If Juju unit topology information is not present in
  alert rules expressions or labels then it is not
  injected by MetricsEndpointProvider.

- If Juju unit topology information is present in
  alert rule expressions or labels then it is not
  removed by  MetricsEndpointProvider.
@balbirthomas
Copy link
Contributor

balbirthomas commented Mar 8, 2022

For whatever it is worth. Here is my analysis of the impacts of the changes introduced by this commit. I am still going through to double check these so take it with a pinch of salt.

  1. JujuTopology.as_promql_label_dict() removes (pop("juju_unit") juju unit
    topology information. as_promql_label_dict(). The impacts of this change are
  • JujuTopology.identifer() uses as_promql_label_dict() - for these impacts
    see point 2 below.

  • JujuTopology.promql_labels() uses as_promql_label_dict() - for these impacts
    see point 3 below.

  1. The JujuTopology.identifer() method previously used as_dict(). Now it
    uses it uses as_promql_label_dict(). as_dict() included juju_unit but
    as_promql_label_dict() does not. The impact of this change is as follows
  • ProviderTopology.scrape_identifier() uses JujuTopology.identifer().
    Previously scrape_identifer() included juju unit now it does not.
    scrape_identifier() is used by MetricsEndpointConsumer to set the
    prefix of the Prometheus scrape job name only for jobs forwarded by
    MetricsEndpointProvider (but not for jobs sent by MetricsEndpointAggregator)

  • The AlertRules._group_name() uses JujuTopology.identier()
    to construct a name for an alert rule group. AlertRules can be instantiated
    with any subclass of JujuTopology. However AlertRules is only used by
    MetricsEndpointProvider and PrometheusRulesProvider but not by
    MetricsEndpointAggregator.

    The alert rule group names constructed by AlertRules will never container
    juju unit regardless of which kind of topology (ProviderTopology or
    AggregatorTopology is used)

  • MetricsEndpointConsumer.alerts() uses JujuTopology.identifer() to build
    an indexed list of alerts, which are indexed using the "identifier". This index
    will no longer contain juju unit topology information, for alert rules from both
    MetricsEndpointProvider and MetricsEndpointAggregator.

  • MetricsEndpointConsumer._static_scrape_config() uses JujuTopology.identifier()
    for constructing the scrape job name on for jobs provided by MetricsEndpointProvider.
    After this commit these job names will not include juju unit topology information.

  1. JujuTopology.promql_labels() previously previously used
    JujuTopology.as_dict() but now it uses JujuTopology.as_promql_label_dict().
    The impact of this change is as follows.
  • JujuTopology.promql_labels() is only used in JujuTopology.render() method
    which is used to inject topology information into an alert rule expression by
    the AlertRules object. Hence AlertRules object will never inject juju unit
    topology into any alert rule read by it.
  1. ProviderTopology.scrape_identifer() now uses JujuTopology.identier() Since
    ProviderTopology previously constructed the identifer without juju unit information
    this change may not have any impacts.

simskij
simskij previously requested changes Mar 8, 2022
Copy link
Member

@simskij simskij left a comment

Choose a reason for hiding this comment

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

once tests pass and the conversations have been properly resolved, I'm happy to approve.

sed-i
sed-i previously approved these changes Mar 8, 2022
@dstathis dstathis dismissed simskij’s stale review March 8, 2022 18:02

All objections resolved

tox.ini Show resolved Hide resolved
rbarry82
rbarry82 previously approved these changes Mar 8, 2022
Copy link
Contributor

@rbarry82 rbarry82 left a comment

Choose a reason for hiding this comment

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

Just quibbling about the alert name. Is the change to tox.ini intentional?

Abuelodelanada
Abuelodelanada previously approved these changes Mar 8, 2022
@dstathis dstathis merged commit f4fdb67 into main Mar 9, 2022
@dstathis dstathis deleted the remove_unit_from_alerts branch March 9, 2022 09:14
sed-i added a commit to canonical/avalanche-k8s-operator that referenced this pull request Mar 10, 2022
sed-i added a commit to canonical/cos-configuration-k8s-operator that referenced this pull request Mar 10, 2022
sed-i added a commit to canonical/prometheus-scrape-config-k8s-operator that referenced this pull request Mar 10, 2022
sed-i added a commit to canonical/cos-lite-bundle that referenced this pull request Mar 11, 2022
This PR adds an itest that addresses canonical/prometheus-k8s-operator#239:
Now that juju_unit is no longer added to rule files, this test confirms that alerts
still fire per unit (not only from the leader unit), and are correctly labeled as such.
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.

7 participants