-
Notifications
You must be signed in to change notification settings - Fork 306
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
TextParser: Tolerate trailing whitespace #33
Comments
Depending on the decision on prometheus/docs#543, it might be alright to not tolerate trailing whitespace. |
Un-assigning from myself as I intend to focus on other Prometheus project. Right now, others are way more involved in text format affairs than I am. |
b990f48 Merge pull request prometheus#42 from kinvolk/lorenzo/fix-git-diff 224a145 Check if SHA1 exists before calling `git diff` 1c3000d Add auto_apply config for wcloud 0ebf5c0 Fix wcloud -serivice 4fe078a Merge pull request prometheus#39 from weaveworks/fix-wrong-subtree-use 3f4934d Remove generate_latest_map 48beb60 Sync changes done directly in scope/tools 45dcdd5 Merge pull request prometheus#37 from weaveworks/fix-mflag-missing b895344 Use mflag package from weaveworks fork until we find a better solution e030008 Merge pull request prometheus#36 from weaveworks/wcloud-service-flags 9cbab40 Add wcloud Makefile ef55901 Review feedback, and build wcloud in circle. 3fe92f5 Add wcloud deploy --service flag 3527b56 Merge pull request prometheus#34 from weaveworks/repo-branch 92cd0b8 [wcloud] Add support for repo_branch option 9f760ab Allow wcloud users to override username 38037f8 Merge pull request prometheus#33 from weaveworks/wcloud-templates 7acfbd7 Propagate the local username e6876d1 Add template fields to wcloud config. f1bb537 Merge pull request prometheus#30 from weaveworks/mike/shell-lint/dont-error-if-empty e60f5df Merge pull request prometheus#31 from weaveworks/mike/fix-shell-lint-errors e8e2b69 integrations: Fix a shellcheck linter error a781575 shell-lint: Don't fail if no shell scripts found db5efc0 Merge pull request prometheus#28 from weaveworks/mike/add-image-tag 5312c40 Import image-tag script into build tools so it can be shared 7e850f8 Fix logs path dda9785 Update deploy api f2f4e5b Fix the wcloud client 3925eb6 Merge pull request prometheus#27 from weaveworks/wcloud-events 77355b9 Lint d9a1c6c Add wcloud events, update flags and error nicely when there is no config git-subtree-dir: tools git-subtree-split: b990f488bdc7909b62d9245bc4b4caf58f5ae7ea
…clearer Use orgID and userID for the user context
I just ran into this. The error message with a trailing space is really confusing. It says |
I don't think we'll be making any changes here, if you use a client library it'll handle it for you plus OpenMetrics will supersede all this. |
I guess a typical scenario where to run into this is the textfile collector of the node exporter. That's where all kind of whitespace usage will be encountered because the textfile snippets are often created in a makeshift way. From that perspective, I see value in allowing trailing whitespace. A different story is if and when the textfile collector will switch to OpenMetrics. If it does, we'd impose the strict whitespace requirement of OpenMetrics to all the creators of textfile snippets. (In principle, I'd love to let the textfile collector converge on OpenMetrics, too, but I'd also prefer OpenMetrics to be liberal with whitespace, which isn't going to happen.) |
My thoughts are that changing the Prometheus text format this late in the game is unwise, anything that it currently does is now defacto the standard. Keep in mind that there are parsers other than the ones we maintain, and this would be a breaking change. |
We wouldn't change the creation code, we would only make the parser more tolerant. It doesn't have to be part of the standard, it's just following the https://en.wikipedia.org/wiki/Robustness_principle . Obviously, it would be bad if we saw the parser as a test to certify standard compliance of Prometheus text format generators. I wouldn't see it that way, simply because the Prometheus text format is not a publicly advertised standard. (That would be OpenMetrics, for which those tests exist or will exist.) |
This code is the official parser. If it parses it, it's in compliance.
Thus that would be changing the standard.
This turns out to be a bad idea, due to things like this.
I don't see how that's relevant. We've always defined this code to be the reference implementation. |
I don't follow that line of argument. It all hinges on the premise that the parser is a compliance test and that the parser code defines the standard. For better or worse, that was never standardized or advertized. We are in a gray area here. I would like to navigate this ambiguity to the benefit of our users. In this case, allowing trailing whitespace is what most people will expect (as proved by multiple people that have run into this). We'll help all of them. The counter case to bring up here is that somebody might have implemented a parser that doesn't tolerate trailing whitespace, too. If now somebody creates text format with trailing whitespace that then somehow makes it to that 3rd party parser, there will be a problem. That's a lot of If's:
|
It's been stated multiple times over the years. For example if promtool (which uses this) parses your /metrics then it is valid. It has always been the reference implementation.
This describes both the current version of this parser and the parser inside Prometheus. Differing versions of Prometheus, the node exporter or pgw doing different things for what is nominally all the v0.0.4 format is not desirable behaviour. We might have gotten away with this a few years back, but today changing this would require defining a v0.0.5 - which I don't think we're going to do.
Also as far as I'm aware we've only ever had one report related to this, for a user who has (presumably) long resolved their issue. It seems a bit disproportionate to introduce such risk/complexity for a use case that effectively doesn't exist. It's not practical to change the format every time a user comes up with a new form of invalid input. |
Most people who run into this issue will probably realize after a while that they made a mistake. Very few will come here and complain. It is still surprising that no trailing whitespaces are accepted, which is made worse by the confusing error message. I would expect many brain cycles of humanity wasted on this. If the Prometheus server rejected trailing whitespace, then fixing this issue would indeed also require changing Prometheus because I do think we should be consistent within the project, including the degree of tolerance our parsers apply. I'd see it as an argument to change neither the Prometheus server's behavior nor the text parser at this point. But what if the Prometheus server accepted trailing whitespace? :philosoraptor: |
Our tolerance does vary, the Prometheus one is for speed rather than
correctness, but this is not the sort of we should vary on. What does
Prometheus do currently?
…On Mon 20 Jan 2020, 15:32 Björn Rabenstein, ***@***.***> wrote:
Most people who run into this issue will probably realize after a while
that they made a mistake. Very few will come here and complain. It is still
surprising that no trailing whitespaces are accepted, which is made worse
by the confusing error message. I would expect many brain cycles of
humanity wasted on this.
If the Prometheus server rejected trailing whitespace, then fixing this
issue would indeed also require changing Prometheus because I do think we
should be consistent within the project, including the degree of tolerance
our parsers apply. I'd see it as an argument to not change the Prometheus
server's behavior at this point.
But what if the Prometheus server accepted trailing whitespace?
:philosoraptor:
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33?email_source=notifications&email_token=ABWJG5SQIVLFKSNURAFFHA3Q6W7ZHA5CNFSM4B5OOPP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJNAIPI#issuecomment-576324669>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABWJG5V7YHJC235ZSMDDNBLQ6W7ZHANCNFSM4B5OOPPQ>
.
|
The Prometheus server currently tolerates trailing whitespace. |
That is a problem then. |
Not accepting trailing whitespaces would not have been a big problem if the error message would not have said the exact opposite of what is actually the problem. That was the real problem. |
The official parser and the Prometheus parser doing different things here is something that needs resolving, one way or the other. The Prometheus parser is wrong by the letter of the law, but this could be considered by some to be a breaking change. Improving error messages is also good. |
2.15 and master might differ on this. |
Oh? Is this a recent regression? |
mmh no. I was thinking that maybe the new parser was more strict about this. it is not. |
This is again based on the premise that the IMHO, the letter of the law is https://prometheus.io/docs/instrumenting/exposition_formats/ . It says:
That's crystal clear. The |
Hello, just dropping in here to weigh in. I'm a newly minted prometheus user, though I'm have a fair bit of development experience. Please, allow extra whitespace! 🙏 I'm glad the docs seem to come down on the right side of this. The current behavior is confusing and discouraging. A format in this simple human-readable style sends a strong message that extra whitespace is ignored. At the moment, it's hard to figure it out that it's not. I almost gave up on an exporter script because of this. I'm glad I pushed through. Not everyone will not have the time and energy to do so. This comment is the only reason I considered whitespace was my issue (thanks @beorn7!). A friendly/helpful error message may be good low hanging fruit, but I'd love to see the underlying whitespace problem sorted. Hope this is useful, and let me know if I can help any other way. |
Yes it's a bug because the docs say otherwise as implemented. |
Apparently, trailing whitespace is not tolerated, see prometheus/prometheus#1469 .
The text was updated successfully, but these errors were encountered: