Exemplar label length limit: consider bytes rather than UTF-8 characters

[beorn7]

I'm sure I commented on this at some point while reviewing the spec. But I never filed an issue here because it seemed like one of the nittier-grittier aspects. Looking at discussions like this one, however, I get the feeling that the practical impact might be severe enough to reconsider the current wording is the spec:

"The combined length of the label names and values of an Exemplar's LabelSet MUST NOT exceed 128 UTF-8 characters."

The problem is that a UTF-8 character has variably byte length, from one to four. So this limits the max byte count needed for the label names and values to 512 bytes. Therefore, it is easy to be permissive and just allow a total length of 512 bytes. However, if a permissive system (e.g. Prometheus) ingests a non-compliant exemplar and then wants to propagate it (e.g. via remote-write) to a less permissive system, it will result in a failure to propagate the exemplar. Therefore, it's generally better to have a strict check, but the strict check requires scanning the label names and values to find out how many UTF-8 characters are contained, which is a performance problem in high-ingestion systems.

IIUC the intention of the length limit is to limit the space exemplars will take. My understanding is that limiting the byte length of the UTF-8 strings rather than the character count is better in every relevant aspect.

[brian-brazil]

This is something we've discussed in the past, and the decision was made to use characters rather than bytes as all other parts of the spec are about UTF-8 characters so this avoids confusion. This is also less likely to cause surprise in a setups where non-ASCII charsets are in use. The purpose of the limit is to avoid users going overboard and misusing exemplars for logging or trace data, so increasing the number of ASCII characters allowed would increase the chances of such misuse while putting users of non-ASCII charsets for valid non-trace-id uses at a disadvantage.

Keep in mind that changing this would not save parsers from having to process the entirety of the exemplar character by character, as parsers also need to a) determine where the end of the label value is and b) confirm that the label characters are valid UTF-8. Accordingly there isn't performance micro-optimisation here in practice, as you need to go through character by character in any case.

However, if a permissive system (e.g. Prometheus) ingests a non-compliant exemplar

Invalid input must be rejected by rejecting the whole scrape.

[beorn7]

Keep in mind that changing this would not save parsers from having to process the entirety of the exemplar character by character, as parsers also need to a) determine where the end of the label value is and b) confirm that the label characters are valid UTF-8.

That's true. At least in principle, it's possible to do the length check at the same time as the other validations.

Accordingly there isn't performance micro-optimisation here in practice, as you need to go through character by character in any case.

In practice, programmers will let the lexer split up the string, then call https://golang.org/pkg/unicode/utf8/#Valid on the label value, and then call https://golang.org/pkg/unicode/utf8/#RuneCountInString on both the label value and the label name.
(At least that's what Prometheus is already doing, respectively for the last step, what's planned in prometheus/prometheus#8781 .)

This additional full pass through all label names and values can only be avoided by rather involved optimizations.
Which would be fair if it gave us any benefits. But I don't buy the arguments above why you chose a character limit. Any small limit will effectively prevent users from putting arbitrary trace or log data into the exemplar labels, may it by byte or UTF-8 characters. The latter is IMHO just way more confusing.

[brian-brazil]

In practice, programmers will let the lexer split up the string

This is specific to the implementation you're looking at, it is not a universal.
The reference parser for example doesn't have a lexer.

No matter whether a 128 byte or 128 character limit is chosen, one is going to be easier/faster to implement in one language and one in another language. I don't see a point in changing things just because of how things work in one particular language for one particular implementation to allow for one minor micro-optimisation. Especially when other performance improvements in the same vein are not being considered for that implementation, and such a change would make things harder/slower in other languages and implementations.

Something else to consider is that languages have in general been slowly moving in the direction of working with characters rather than bytes. This help avoid a mix of bytes and characters which tends to cause tons of confusion and bugs in typical use cases (Python being one of the best examples of this), which is why OpenMetrics avoids mixing bytes and characters.

You may not personally buy it but this is the consensus from every time this was discussed over many years, and thus what the spec has always required.

[beorn7]

I admit it's just a small nail, but it's another nail in the coffin. 😞

[juliusv]

I have trouble forming a strong opinion on this either way. At the moment I have a slight tendency to prefer byte length limits exactly for the practical reasons that Björn mentioned, namely that most Go programmers would do this check separately using RuneCountInString(). And even though OM is a standard it seems to be ok to slightly optimize for the most relevant language that will be used for parsing OM for years to come.

[RichiH]

This has been discussed repeatedly, and with largely the same arguments. These are very good arguments both ways.

My own slight(!) leaning was always towards byte limits, but similar to Julius, I find it hard to form a very strong opinion; my main concern is to have a defined way of what MUST be done.

As OM aligns itself closely to Prometheus, if Prometheus were to form a strong opinion, OM would find a way to follow suit. I can put this on the backlog for the next dev summit and ask people on -team to chime in.

[brian-brazil]

As OM aligns itself closely to Prometheus, if Prometheus were to form a strong opinion, OM would find a way to follow suit.

Which part of Prometheus? This change would make code more complicated for at least Python, Ruby, and Java users. For the sake of a net zero-line change to Go users, which is not exactly a piece of code that'll need heavy ongoing maintenance/development.

In terms of performance doing some quick numbers for the Prometheus scrape path this would have less than ~0.3% CPU difference in the worst case where every sample had a typical-length trace exemplar. And that's before doing optimisations, which can get that down to what looks to be 0 if you combine RuneCountInString into ValidString which is trivial as they already have extremely similar code. Even just copying over the fastpath in ValidString to RuneCountInString would bring things down to ~0.07% CPU. And this is all the scrape code path in Prometheus, where CPU is very rarely a concern in terms of resource usage. PromQL tends to dominate in terms of CPU usage, with RAM being far more often the usual concern overall.

The argument here seems quite weak to me. For the sake of a minuscule performance gain which can be readily obtained in other ways, and using one function rather than another in one particular language, we'd be talking making a breaking change to the OM spec and making things notably harder in multiple other languages. As an example it took careful reading of the docs to figure out how you would correctly count bytes in Java, as there's a nasty gotcha there.

[juliusv]

See, now I'm leaning slightly towards keeping the UTF-8 rune length again. Probably if @beorn7 posts another time with more arguments, I will lean in that direction again 😆

Overall that shows me that it's probably not worth changing, so maybe it's fine to keep it as it is unless there's strong new arguments.

[beorn7]

I have plenty of more arguments, but I believe the arguments presented are sufficient. I don't want to widen the debate even more. Furthermore, I have a strong notion of deja-vu with the way this discussion goes. Here and with every single of the many approach I made to salvage OpenMetrics, my impression is that I live on a different planet when it comes to weighing the relevance of different arguments. In the best case, it's me who is completely off. In which case I'm not helpful here. But even if I'm right, I see my attempts as completely moot by now. I tried really hard a lot of times. I hereby give up and withdraw from any discussion around OpenMetrics.

[Forty-Bot]

Hi, I just want to clarify something. In the spec, it says "characters". Do you mean code points? E.g. ü and ü are both the same character, but the latter has two code points and the former has one.

[brian-brazil]

Characters is what is meant.

[Forty-Bot]

Really? But e.g. the go and python prometheus clients measure code points and not characters. The reason I bring this up is that a limit on characters is effectively no limit on the actual size of the encoded string, since you can e.g. export zalgo text which uses hundreds of bytes per character.

[brian-brazil]

Hmm, in that case code points probably makes more sense. Tbh if it matters then the user is probably misusing exemplars.

…

On Sun 23 Jan 2022, 16:13 Forty-Bot, ***@***.***> wrote: Really? But e.g. the go <https://github.com/prometheus/client_golang/blob/22da9497b8f0d53072dfc4721904faa7395d8318/prometheus/value.go#L201> and python <https://github.com/prometheus/client_python/blob/ff1960462e853d46ede35b661ccf9636c89f5adb/prometheus_client/metrics.py#L52> prometheus clients measure code points and not characters. The reason I bring this up is that a limit on characters is effectively no limit on the actual size of the encoded string, since you can e.g. export zalgo text which uses hundreds of bytes per character. — Reply to this email directly, view it on GitHub <#198 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWJG5WCH3DZZM5RLYLY4VLUXQSKDANCNFSM44C7QT2A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>