Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: remove leading and trailing zeros #119

Merged
merged 7 commits into from
Feb 2, 2021
Merged

feat: remove leading and trailing zeros #119

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cd9e295
feat: remove leading and trailing zeros
Yiyiyimu Feb 1, 2021
888b3a2
fix CI
Yiyiyimu Feb 1, 2021
a7f019f
could not pass CI, for count/sum when there is no 'le', the 'stale' w…
Yiyiyimu Feb 1, 2021
63ef992
check mark before 'le'
Yiyiyimu Feb 1, 2021
0f48d3c
fix according to review
Yiyiyimu Feb 2, 2021
1f79729
tonumber() would have different output in lua5.3 so revert to manual …
Yiyiyimu Feb 2, 2021
f3aa27c
add zero before decimal point when needed
Yiyiyimu Feb 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions prometheus.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,39 @@ local function construct_bucket_format(buckets)
return "%0" .. (max_order + max_precision + 1) .. "." .. max_precision .. "f"
end

-- Format bucket format when expose metrics.
--
-- This receives a key, remove leading and trailing zeros and return the cleaner
-- number, in order to make the output more clear, without effect on the increasing
-- order.
--
-- Args:
-- key: the metric key
--
-- Returns:
-- (string) the formatted key
local function format_bucket_when_expose(key)
local all_le_string = string.match(key, '(le=".*")')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=")(.*)(")')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

And thanks for the regex suggestions🆒

local part1, bucket, part2 = string.match(key, '(le=")(.*)(")')
if tonumber(bucket) == nil then
return key
Yiyiyimu marked this conversation as resolved.
Show resolved Hide resolved
end

--remove leading zeros
local _
_, bucket = string.match(bucket, '(0*)(.*)')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


--remove trailing zeros and decimal point
while (bucket:sub(-1) == "0") do
bucket = bucket:sub(1, -2)
end
if (bucket:sub(-1) == ".") then
bucket = bucket:sub(1, -2)
end

return key:gsub(all_le_string, table.concat({part1, bucket, part2}, ""))
end

-- Return a full metric name for a given metric+label combination.
--
-- This function calculates a full metric name (or, in case of a histogram
Expand Down Expand Up @@ -806,6 +839,7 @@ function Prometheus:metric_data()
end
seen_metrics[short_name] = true
end
key = format_bucket_when_expose(key)
-- Replace "Inf" with "+Inf" in each metric's last bucket 'le' label.
if key:find('le="Inf"', 1, true) then
key = key:gsub('le="Inf"', 'le="+Inf"')
Expand Down
6 changes: 3 additions & 3 deletions prometheus_test.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,10 +607,10 @@ function TestPrometheus:testCollect()

assert(find_idx(ngx.printed, "# TYPE b1 histogram") ~= nil)
assert(find_idx(ngx.printed, "# HELP b1 Bytes") ~= nil)
assert(find_idx(ngx.printed, 'b1_bucket{var="ok",le="0100.0"} 2') ~= nil)
assert(find_idx(ngx.printed, 'b1_bucket{var="ok",le="100"} 2') ~= nil)
assert(find_idx(ngx.printed, 'b1_sum{var="ok"} 5250') ~= nil)

assert(find_idx(ngx.printed, 'l2_bucket{var="ok",site="site2",le="04.000"} 2') ~= nil)
assert(find_idx(ngx.printed, 'l2_bucket{var="ok",site="site2",le="4"} 2') ~= nil)
assert(find_idx(ngx.printed, 'l2_bucket{var="ok",site="site2",le="+Inf"} 4') ~= nil)

-- check that type comment exists and is before any samples for the metric.
Expand Down Expand Up @@ -647,7 +647,7 @@ function TestPrometheus:testCollectWithPrefix()

assert(find_idx(ngx.printed, "# TYPE test_pref_b1 histogram") ~= nil)
assert(find_idx(ngx.printed, "# HELP test_pref_b1 Bytes") ~= nil)
assert(find_idx(ngx.printed, 'test_pref_b1_bucket{var="ok",le="0100.0"} 2') ~= nil)
assert(find_idx(ngx.printed, 'test_pref_b1_bucket{var="ok",le="100"} 2') ~= nil)
assert(find_idx(ngx.printed, 'test_pref_b1_sum{var="ok"} 5250') ~= nil)
end

Expand Down