-
Notifications
You must be signed in to change notification settings - Fork 694
feat: Implement utf8 label name client capability #4442
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have todos like tests and more api coverage (I'm missing some read path APIs), but wanted to get this draft out for comment sooner rather than later
This feature is a relaxation in previous validation for label names. Given the backward incompatible nature of this change (for example, PromQL queries must be [updated to support utf-8 label names](https://prometheus.io/docs/guides/utf8/#querying)), it is gated behind a "client capability": a key/val that a client must set in the `Accept:` header.
6f6e55d
to
72a3b80
Compare
- Moved all client capability logic to its own module - Move legacy (i.e. non utf-8 enabled) validation logic to query path - Implemented in v2 LabelNames and Series APIs - Relax write path to allow any utf-8 valid label names - Created HTTP and gRPC middleware for client capability - Updates og UI to work with utf-8 label name selection - Removed utf8 label name from feature flags [since unrelated to feature flags now]
I've verified
Breaking changes
Open questions cc @simonswine @bryanhuhta
TODOs
|
- General cleanup - Removed clientcapabilities package, moved back in to frontend package - Using struct for client capabilities in the context - Added many tests
TL;DR: Worth mentioning that recording rules are not 100% compatible with UTF-8. We should validate exported label names and metric name. Extended: From recording rules model https://github.com/grafana/pyroscope/blob/main/api/settings/v1/recording_rules.proto#L56-L92:
I propose to validate those so we make sure our rules are ok. We have 2 places to validate this, first at recording rule creating time (UI sending a POST request to tenant-settings), and compaction workers fetching rules from tenant-settings at compaction time, basically wrapped in this constructor. Maybe you find that some of the requirements already meet (I remember using some validations for the metric_names, for example). |
Current State of the PR
Known Issues
Proposed ModificationChange writes to also respect the
|
My two cents about it: While client capabilities provide granular control, I think we should keep a server-side feature flag as a kill switch ideally to allow us to:
I think the v1 APIs should be supported because:
I'm not sure about the best approach in a mixed-data scenario... I see:
I believe the key decision is whether we want a clean migration (store UTF-8 only, handle compatibility at read/query time) or safer migration (store based on client capability, accept permanent mixed data); do we want to support both formats coexisting at the same time for a given user...? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took a while to come back to you on this.
Let me address from you commentt
Not implemented for v1 APIs
As per marc's comment, this needs to be implemented in both implementations. (Note: There is not necessarily a v1/v2 API, but v1/v2 have different code paths implementing the same API.
Mixed read/write semantics during rollout
I think during version rollout of the new version of Pyroscope small inconsistencies are acceptable, also even if the rollout would be instant, there is still old data that will have complied to the old push validation. I think this is the price we will have to for not implementing a full forward/backward compatibility of labels names outside of the legacy validation.
Label selectors and compatibility
I agree this is a problem and there are further problem with translating label names on the read path (translation is not reversible so follow upcall would need to implement them too (all calls with selector), therefore we said in https://github.com/grafana/pyroscope-squad/issues/434#issuecomment-3113813464, we don't do such translation and filter non legacy compatible names in Series/LabelNames call.
So what I am proposing is to change the PR to:
- Filter on read (not Sanitize on read)
- Scope this PR to be backend and read path only (both v1/v2 implmenetation of the frontend)
- Create separate new PR for the frontend to use the client capabilities defined here
- Create separate new PR that modfies the write path to no longer sanitise label names, make this controlable on a per tenant basis and publish the state of it through a new feature flag, something like
utf8LabelNamesWritePath
public/app/components/TagsBar.tsx
Outdated
// Identifies whether a label is in a query or not | ||
function isLabelInQuery(query: string, label: string, labelValue: string) { | ||
return query.includes(`${label}="${labelValue}"`); | ||
return query.includes(`"${label}=${labelValue}"`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do this in separate PR to keep the scope as tight as possible.
I also think this would be the correct way to double-quote utf8 names in selectors:
return query.includes(`"${label}=${labelValue}"`); | |
return query.includes(`"${label}"="${labelValue}"`); |
// TODO add metrics = # requests like this and # clients [need | ||
// labels for requests and clients/tenet and user agent(?)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics like this are likely very costly (high cardinality), so I would advise against it. If we need anything like this we already should log those headers. (Maybe doublecheck that works as exptected, but we are setting -server.log-request-headers
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm considering adding a counter metric with low cardinality labels (tenant + capability name).. any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's makes a lot of sense, probably a good follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed up here: #4498
- Removes profilecli and OG front end changes (will go in separate PR's) - Updates read path to filter instead of sanitize on read - Reverts utf8 featureflag removal (will be used in separate PR)
Taking this PR out of draft mode 🎉 This PR implements a client capability framework, with This PR does not provide full utf-8 label name support. Instead, it offers the first pass at what will be full support, with follow up PRs to provide full support (expanded on below). This PR respects the A future PR will disable the write path label name sanitization if the Another future PR will update the OG UI and profilecli clients to take advantage of this client capability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think this is very close to LGTM, a couple of suggestions, after that I am happy for you to merge this.
pyroscope/pkg/settings/recording/recording.go Lines 279 to 290 in e0e688b
@alsoba13 If I'm understanding the code correctly, it seems like label names from recording rules are currently utf-8 validated (because of this code), and not validated by Prometheus' legacy validation. I can address this issue, but in a separate PR since it's orthogonal to label name compatibility for profiles. |
for _, name := range toFilter { | ||
if _, _, ok := validation.SanitizeLegacyLabelName(name); !ok { | ||
level.Debug(q.logger).Log("msg", "filtering out label", "label_name", name) | ||
continue | ||
} | ||
filtered = append(filtered, name) | ||
} | ||
return filtered, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary, as LabeNames
should already filter them with the same logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic is required when len(req.Msg.LabelNames) != 0
; will add for that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR implements a client capability framework, with allow-utf8-labelnames as the first capability. Capabilities are flags clients set in the Accept header to inform the API about specific client support. This is useful for cross-API features (like utf-8 support).
This PR does not provide full utf-8 label name support. Instead, it offers the first pass at what will be full support, with follow up PRs to provide full support (expanded on below).
This PR respects the allow-utf8-labelnames client capability in the read path (both v1 and v2 LabelNames and Series APIs). If the capability is not set or not set to true, these APIs will filter out "invalid" label names (i.e. with chars outside of [a-zA-Z0-9_.]) from the response. This logic is currently a no-op, given it's not yet possible to write label names outside of this charset.
A future PR will disable the write path label name sanitization if the allow-utf8-labelnames client capability (gated behind a per-tenant feature flag).
Another future PR will update the OG UI and profilecli clients to take advantage of this client capability.
cc @simonswine @bryanhuhta