expfmt: throw error when metric's name not provided instead panicing#811
expfmt: throw error when metric's name not provided instead panicing#811Alhanaqtah wants to merge 1 commit intoprometheus:mainfrom
Conversation
|
@roidelapluie could you review PR, please?) |
|
This change looks good to me. Can you add a test to exercise this case so we can catch regressions? And can you sign your commit to pass the DCO check? Thanks! |
|
Hi, I would like to do a release of prom/common and this would be a nice fix to include, do you think you can add the test and fix DCO soon? |
Signed-off-by: Banana Duck <a.usama@yandex.ru>
cfaa159 to
ec048aa
Compare
I've done everything. You can merge it) P.S. |
|
@ywwg Could you merge PR, please?) |
|
Hello! I would be glad if someone of you approved PR) |
jan--f
left a comment
There was a problem hiding this comment.
Apologies for the delay here.
I don't think this is quite correct. This test case already exists at https://github.com/prometheus/common/pull/811/files#diff-f94ed3d7ef59cf8de9ae97ae9e654cadda29ab3257af8a5112d93e2290e6bad2R804, though without HELP and TYPE annotation. In these lines though you do pass a metric name.
On current main this new test case no longer causes a panic, so I assume this was fixed elsewhere and the parser can take the name from the HELP/TYPE lines. Without those lines the parser returns the expected parsing error.
|
It is entirely possible though that this new test case covers a corner case in the parser we need to fix. I reached out for more eyes on this. |
jan--f
left a comment
There was a problem hiding this comment.
So yes this is a corner case previously not tested for, thanks for fixing this.
Can you please rebase on current main?
| # HELP backupmonitor_size The size of the given backup. | ||
| # TYPE backupmonitor_size counter | ||
| {host="local", dir="alpha"} 1834194837 | ||
| {host="remote", dir="beta"} 133638016 |
There was a problem hiding this comment.
| # HELP backupmonitor_size The size of the given backup. | |
| # TYPE backupmonitor_size counter | |
| {host="local", dir="alpha"} 1834194837 | |
| {host="remote", dir="beta"} 133638016 | |
| # HELP metric a metric. | |
| # TYPE metric counter | |
| {label="bla"} 3.14 |
Just to be consistent with previous test cases.
This PR enhances extfmt behavior when incorrect metrics privided by user — metrics without name. Now
TextParserthrows error "invalid metrics name" instead of panicing (#3374)@roidelapluie