feat: remove leading and trailing zeros

[Yiyiyimu]

Signed-off-by: yiyiyimu wosoyoung@gmail.com

fix #118

Some transformation examples:

00.750 -> .75
01.000 -> 1
01.010 -> 1.01

One question: do we need a switch for this transformation? Then maybe we need to pass come config when init lua-prometheus. Considering prometheus library in other language (golang, python) does not add leading and trailing zeros, maybe we could just directly remove them.

[coveralls]

Coverage decreased (-0.1%) to 94.758% when pulling f3aa27c on Yiyiyimu:remove-zeros into 9308961 on knyar:master.

[dolik-rce]

This will remove all leading zeros (e.g. 00.01 -> .01) you'd probably want to leave one before decimal point (0.01).

Actually, I think the simplest way to do the number formatting here would be bucket = tostring(tonumber(bucket)). This will take care of both leading and trailing zeros. I'm not 100% sure about speed, but I think it won't be bad at all, because both functions are probably implemented in C and will be faster than using patterns and iterating strings in Lua.

[Yiyiyimu]

Yeah that's right! That is faster (regex in pure lua is indeed pretty slow) and way more elegant. Thanks a lot!

[dolik-rce]

This will probably work correctly only if le is the last label. Otherwise, it'll match too much. E.g.: for key='my_metric{le="0.10",z="x"}' it will return 'le="0.10",z="x"'.

Also, it might match incorrectly if there were a label ending with 'le', e.g.: my_metric{stale="0",le="0.01"}.

I think this function deserves more thorough unit tests...

[Yiyiyimu]

Actually le would always be the last label in the current implementation, see

nginx-lua-prometheus/prometheus.lua

Lines 323 to 333 in 499de49

	local bucket_pref
	if self.label_count > 0 then
	-- strip last }
	bucket_pref = self.name .. "_bucket" .. string.sub(labels, 1, #labels-1) .. ","
	else
	bucket_pref = self.name .. "_bucket{"
	end
	for i, buc in ipairs(self.buckets) do
	full_name[i+2] = string.format("%sle=\"%s\"}", bucket_pref, self.bucket_format:format(buc))
	end

so I think we don't need to worry about that.

But the later one is what I missed before. Thanks for remind!

I tried to get the last le=, but when talking about sum/count, there would be no label le but the others say stale, so get the last one is not appropriate. Right now I fix this problem by filter both ,le=" and {le=". Do you have some other suggestions?

And new unit tests added for 1. label "stale" 2. no labels.

[dolik-rce]

Oh, I didn't check the implementation details. So it is probably ok to do it this way, but this assumption should be checked in unit tests (so it doesn't break in future, if someone changes the ordering of labels).

Regarding the checking: You can match both variants at once:

local part1, bucket, part2 = key:match('([,{]le=")(.*)(")')

[Yiyiyimu]

Yes I've added unit tests for these scenarios, thanks for remind!

And thanks for the regex suggestions🆒

[knyar]

Thank you for preparing this PR! Note that tests seem to be failing on lua 5.3 for some reason.

[Yiyiyimu]

Thank you for preparing this PR! Note that tests seem to be failing on lua 5.3 for some reason.

Got it. It's pretty annoying that tonumber would have different output between different lua version (Ref)

> $ lua5.1 -e 'print(tonumber("9.0"))'
> 9
> $ lua5.3 -e 'print(tonumber("9.0"))'
> 9.0

Fixed with another check for the .0 tail

[dolik-rce]

Well, the behaviour of Lua 5.3 is kind of inconsistent ☹️

tostring(tonumber("0100")) --> "100"
tostring(tonumber("0100.0")) --> "100.0"

Since there is only one type for integers and floats and those numbers are equal (tonumber("0100") == tonumber("0100.0") returns true), I'd expect that both would be converted to same string... Almost looks like a bug.

Anyway @Yiyiyimu, your solution (stripping trailing ".0") is OK, I can't think of anything better.

[Yiyiyimu]

Hi @dolik-rce do I need to worry about the coverage check?

[dolik-rce]

Hi @dolik-rce do I need to worry about the coverage check?

That's actually a question for @knyar, he's the maintainer. I'm just an interested bystander 😁

[Yiyiyimu]

That's actually a question for @knyar, he's the maintainer. I'm just an interested bystander 😁

My bad I thought you also help to maintain the project🤣
You are so nice!! 🌹 Thanks again for the review!!

[knyar]

Thanks a lot for preparing this PR and for addressing the comments! I'll wait for a few days and will cut a new release that includes this.

[Yiyiyimu]

@knyar cool! plz ping me when the new release got published~

[knyar]

Anyway @Yiyiyimu, your solution (stripping trailing ".0") is OK, I can't think of anything better.

Actually it seems that ngx_lua always uses lua 5.1, so there is no need to support 5.3. I have removed trimming of .0 in 857e1d9 to make the code simpler.

[Yiyiyimu]

Actually it seems that ngx_lua always uses lua 5.1, so there is no need to support 5.3. I have removed trimming of .0 in 857e1d9 to make the code simpler.

Oh yes you're right! BTW we could also remove Travis CI for lua 5.2

[knyar]

@knyar cool! plz ping me when the new release got published~

@Yiyiyimu I have just released 0.20210206. Thanks again for your contribution!