Repository navigation
Conversation
Signed-off-by: Raphael Skuza <[email protected]>
| name_validation_scheme: utf8 | ||
| ``` | ||
|
|
||
| ### Impact on Cortex components |
There was a problem hiding this comment.
Since the Distributor is a sub-item, it would be better to increase to the ##.
| ### Impact on Cortex components | |
| ## Impact on Cortex components |
There was a problem hiding this comment.
Typo, thank you for pointing it out
|
@RaphSku |
| * [FEATURE] Ingester: Add experimental active series queried metric. #7173 | ||
| * [FEATURE] Update prometheus Alertmanager version to v0.31.1 and add new integration to IncidentIO and Mattermost. #7092 #7267 | ||
| * [FEATURE] Tenant Federation: Add experimental support for partial responses using the `-tenant-federation.allow-partial-data` flag. When enabled, failures from individual tenants during a federated query are treated as warnings, allowing results from successful tenants to be returned. #7232 | ||
| * [ENHANCEMENT] Docs: Guide on UTF-8 Support in Cortex #7091 |
There was a problem hiding this comment.
I don't think it's necessary to leave it in the changelog.
Signed-off-by: Raphael Skuza <[email protected]>
I've added the mentioned components to the guide, thank you for the review. If there is something in particular I should mention about either component, let me know. |
|
@RaphSku
Adding a few short YAML or PromQL snippets based on our tests would perfectly bridge the gap between the configuration and actual usage. |
…tilize UTF-8 support Signed-off-by: Raphael Skuza <[email protected]>
|
@SungJin1212 Thank you again for your comment, totally valid to have such a section and a good idea. I have included it in the latest commit. |
| series.1{"test.utf8.metric"="😄"} | ||
| ``` | ||
|
|
||
| Note, that the metric name `series.1` is not valid under the `legacy` validation scheme. It is accepted only when UTF-8 validation is enabled. This is useful when metric or label names contain characters, such as `.`, that are supported by UTF-8 validation but not by the legacy validation scheme. |
There was a problem hiding this comment.
| Note, that the metric name `series.1` is not valid under the `legacy` validation scheme. It is accepted only when UTF-8 validation is enabled. This is useful when metric or label names contain characters, such as `.`, that are supported by UTF-8 validation but not by the legacy validation scheme. | |
| Note that the metric name `series.1` is not valid under the `legacy` validation scheme. It is accepted only when UTF-8 validation is enabled. This is useful when metric or label names contain characters, such as `.`, that are supported by UTF-8 validation but not by the legacy validation scheme. |
Signed-off-by: Raphael Skuza <[email protected]>
|
@friedrichg friendly ping :) |
sandy2008
left a comment
There was a problem hiding this comment.
Thanks, the earlier review points are all in. I checked the examples and the component notes against the code: two of the snippets don't parse, and the Alertmanager and notifier paragraphs describe a different mechanism from what the code does. Details inline.
| PromQL queries support UTF-8 metric names and label names. For example, a UTF-8 metric name can be queried with the following expression: | ||
|
|
||
| ```promql | ||
| series.1{"test.utf8.metric"="😄"} |
There was a problem hiding this comment.
This doesn't parse: the vendored PromQL parser stops at .1 (1:7: parse error: unexpected number ".1"). A metric name that isn't a legacy identifier has to go inside the braces as a quoted string, which is also what integration/utf8_test.go queries:
| series.1{"test.utf8.metric"="😄"} | |
| {"series.1", "test.utf8.metric"="😄"} |
There was a problem hiding this comment.
You are totally right, my mistake.
|
|
||
| ```yaml | ||
| matchers: | ||
| - "🙂.utf8.label" |
There was a problem hiding this comment.
As written this isn't valid YAML (the list item is a bare string followed by value/isRegex keys); each matcher needs a name field, like the silence the integration test creates:
| - "🙂.utf8.label" | |
| - name: "🙂.utf8.label" |
There was a problem hiding this comment.
Yes, for some reason forgot to include name, thank you for pointing out.
|
|
||
| Ruler external labels are also validated with the selected scheme. With `legacy`, external label names must follow the legacy validation rules. With `utf8`, external label names that are valid under the UTF-8 validation scheme can be used. | ||
|
|
||
| The Ruler also passes the configured validation scheme to the notifier that sends alerts to Alertmanager. As a result, alert labels handled by the notification path are validated according to the selected scheme before alerts are sent. |
There was a problem hiding this comment.
I don't think this validates alert labels: the vendored notifier.NewManager only uses the scheme as the default for alert relabel configs that don't set their own, and the Cortex ruler doesn't configure any. I'd drop this paragraph.
There was a problem hiding this comment.
Dropped, now it should be correct since rule definitions are validated in /pkg/ruler/manager.go and external labels in /pkg/util/validation/limits.go. I hope I didn't miss anything else. Thank you for the information regarding notifier.NewManager.
|
|
||
| ### Alertmanager | ||
|
|
||
| The `name_validation_scheme` setting is not applied directly by Cortex [Alertmanager](https://cortexmetrics.io/docs/architecture/#alertmanager) in the same way it is applied by the Distributor or Ruler. Its effect on Alertmanager is indirect. Alerts sent by the Ruler pass through the Ruler notifier, which is created with the configured validation scheme. As a result, Alertmanager may receive alert labels that were validated according to the selected scheme. |
There was a problem hiding this comment.
This one is applied directly: initAlertManager maps name_validation_scheme to Alertmanager's matcher mode (legacy to classic-mode, utf8 to utf8-strict-mode) and calls compat.InitFromFlags, which decides how matchers in the Alertmanager configuration are parsed and how label names such as group_by entries are validated. So the Alertmanager examples above only work with utf8, and switching to utf8 moves existing Alertmanager configs to the UTF-8 matcher parser with no classic fallback, which seems worth a line in the rollout notes.
There was a problem hiding this comment.
You are right, thank you for pointing that out, rewritten that section and improved recommended rollout strategy section.
- Fixing PromQL expression since it doesn't parse. - Matchers missed "name" key and converted to JSON to better represent the API body. - Removed ruler paragraph claiming validating alert labels. - Correcting Alertmanager section since `name_validation_scheme` is directly mapped to Alertmanager's matcher modes. - Improving recommended rollout strategy. Signed-off-by: Raphael Skuza <[email protected]>
|
@sandy2008 Thank you for your detailed review, all very good points. I have tried to include them and improve/fix the guide. |
What this PR does:
Introduces a Guide on how to setup UTF-8 mode in Cortex
Which issue(s) this PR fixes:
Fixes #7091
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]