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

pkg/ruler: Add external_labels option #4499

Merged
merged 2 commits into from
Mar 7, 2022
Merged

pkg/ruler: Add external_labels option #4499

merged 2 commits into from
Mar 7, 2022

Conversation

BenoitKnecht
Copy link
Contributor

What this PR does:

Add an external_labels option for the ruler, to tag all alerts emitted by the ruler with a given set of labels.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated

@BenoitKnecht
Copy link
Contributor Author

Could someone review this PR and maybe approve the workflow to let the jobs run? Maybe @jtlisi, as it looks like you've contributed most of the notifier code.

The goal here is to be able to specify external labels in Loki, so that all of the alerts it sends are tagged with a given set of labels, mimicking the behavior of Prometheus. This is very useful when running several instances of Loki in different regions or clusters, making the alerts they emit easily identifiable.

@BenoitKnecht
Copy link
Contributor Author

@bboreham I'm sorry to ping people at random, but I'd like at least to get the workflow approved to get the CI running, and if possible some feedback on the proposed changes. I'm happy to rework it if needed; being able to set external labels in Loki is really important for our use-case.

@stale
Copy link

stale bot commented Jan 31, 2022

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 31, 2022
@stale stale bot closed this Feb 20, 2022
@BenoitKnecht
Copy link
Contributor Author

I'm still hoping to get someone to take a look at this PR, as this feature is sorely needed.

@alanprot
Copy link
Member

This seems a valid use case.

It would be nice to configure it per rule as well, no?

@alanprot alanprot reopened this Feb 23, 2022
@stale stale bot removed the stale label Feb 23, 2022
@BenoitKnecht
Copy link
Contributor Author

Hi @alanprot! I'm not sure I get what you mean, labels can already be set on individual rules. The goal of external_labels is to replicate the behavior of external_labels in Prometheus, which is global to the Prometheus instance in that case.

Anyway, the PR is now at grafana/loki#5450, if you want to continue the discussion there.

@BenoitKnecht
Copy link
Contributor Author

(Actually, you may want to merge this here too; let me know if that's the case, I'll make the necessary adjustments.)

bboreham
bboreham approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Sorry I missed this; it seems a nice PR, simple and well-constructed.
One question on the description.

CHANGELOG.md Outdated
@@ -23,6 +23,7 @@
* [FEATURE] AlertManager: Add support for SNS Receiver. #4382
* [FEATURE] Distributor: Add label `status` to metric `cortex_distributor_ingester_append_failures_total` #4442
* [FEATURE] Queries: Added `present_over_time` PromQL function, also some TSDB optimisations. #4505
* [FEATURE] Ruler: Add `external_labels` option to tag all alerts with a given set of labels.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it only alerts, or recording rules too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, this only applies to alerts. It gets passed to the notifier configuration specifically, and I don't see any mention of external labels in vendor/github.com/prometheus/prometheus/rules/recording.go.

The Prometheus documentation also makes it clear that they apply when communicating with external systems:

The labels to add to any time series or alerts when communicating with external systems (federation, remote storage, Alertmanager).

Copy link
Member

Choose a reason for hiding this comment

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

Can you move the changelog to ## master / unreleased

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@BenoitKnecht
Copy link
Contributor Author

So the documentation generation is failing, but it's a bit tricky to resolve.

The first issue is that label.Labels is not recognized. Adding a case "labels.Label to getFieldType() is easy enough, but then it gets rendered as

[external_labels: <list of label> | default = ]

because labels.Labels is defined as []Label; however, it is actually implemented as a map, so external_labels should take the form of

external_labels:
  name: value
  foo: bar

I can try and get around this by defining case "labels.Labels" instead (note the s at the end), but then I get something like

[external_labels: <labelname: labelvalue> | default = ]

which isn't perfect either. The way I defined it manually (which is how it's defined in the Prometheus documentation is

external_labels:
    [ <labelname>: <labelvalue> ... ]

Finally, the other issue I had to get around is the fact that go run ./tools/doc-generator expects a CLI flag for every option, but I didn't define one for external_labels, mainly because it's a bit difficult to translate an option that takes a map as a value into a practical command-line flag.

Any advice on how to handle both of these issues?

@alanprot
Copy link
Member

Humm..

Seems that the best way would be following the other maps?

[external_labels: <map of string to string> | default = ]

@BenoitKnecht
Copy link
Contributor Author

@alanprot Yeah, that makes a lot of sense. :)

I also found the doc:"nocli" option, which allowed the documentation generator to ignore the lack of CLI flag associated with external_labels. This should fix the job that was failing.

@BenoitKnecht
pkg/ruler: Add external_labels option
abd5a6e 
This option can be used to add a set of labels to all alerts emitted by the
ruler.

Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>
@BenoitKnecht
docs/configuration: Document external_labels option
0bc1ebd 
Let the documentation generator understand the `labels.Labels` field, and
generate `config-file-reference.md` with

```console
$ go run tools/doc-generator \
> docs/configuration/config-file-reference.template > docs/configuration/config-file-reference.md
```

Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>
@alanprot alanprot merged commit 45f043f into cortexproject:master Mar 7, 2022
@alanprot
Copy link
Member

alanprot commented Mar 7, 2022

Thanks

@friedrich-at-adobe friedrich-at-adobe mentioned this pull request May 10, 2022
Merged
3 tasks
alanprot pushed a commit that referenced this pull request May 11, 2022
@friedrich-at-adobe
Include changelog number for #4499 #4601 #4707 #4708 (#4737)
454da6c 
Signed-off-by: Friedrich Gonzalez <fgonzale@adobe.com>
@friedrichg friedrichg mentioned this pull request Apr 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanprot alanprot alanprot approved these changes

@bboreham bboreham bboreham approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BenoitKnecht @alanprot @bboreham