-
Notifications
You must be signed in to change notification settings - Fork 23
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
add an efficient labels serializer #474
base: master
Are you sure you want to change the base?
Changes from all commits
725b56c
7765182
0aeaf76
e0fd167
2a1901c
270cbe5
1f6d58f
961ec61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,6 +9,8 @@ local Gauge = require('metrics.collectors.gauge') | |||||
local Histogram = require('metrics.collectors.histogram') | ||||||
local Summary = require('metrics.collectors.summary') | ||||||
|
||||||
local serializers = require("metrics.serializers") | ||||||
|
||||||
local registry = rawget(_G, '__metrics_registry') | ||||||
if not registry then | ||||||
registry = Registry.new() | ||||||
|
@@ -140,4 +142,5 @@ return { | |||||
unregister_callback = unregister_callback, | ||||||
invoke_callbacks = invoke_callbacks, | ||||||
set_global_labels = set_global_labels, | ||||||
labels_serializer = serializers.basic_labels_serializer | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
local clock = require('clock') | ||
local fiber = require('fiber') | ||
local log = require('log') | ||
local serializers = require("metrics.serializers") | ||
|
||
local Shared = {} | ||
|
||
|
@@ -47,12 +48,11 @@ function Shared.make_key(label_pairs) | |
if type(label_pairs) ~= 'table' then | ||
return "" | ||
end | ||
local parts = {} | ||
for k, v in pairs(label_pairs) do | ||
table.insert(parts, k .. '\t' .. v) | ||
-- `label_pairs` provides its own serialization scheme, it must be used instead of default one. | ||
if label_pairs.__metrics_serialize then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it's not a metamethod -- it's just some internal API (even though object is internal by itself). Since it is not a metamethod, I think you shouldn't use double underscore in the name: use |
||
return label_pairs:__metrics_serialize() | ||
end | ||
table.sort(parts) | ||
return table.concat(parts, '\t') | ||
return serializers.default_labels_serializer(label_pairs) | ||
end | ||
|
||
function Shared:remove(label_pairs) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,8 @@ function export.configure_default_collector(...) | |
export.set_default_collector(export.build_default_collector(...)) | ||
end | ||
|
||
local labels_serializer = metrics_api.labels_serializer({ "path", "method", "status" }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would likely break using the global labels. |
||
|
||
--- Measure latency and invoke collector with labels from given route | ||
-- | ||
-- @tab collector | ||
|
@@ -86,11 +88,11 @@ function export.observe(collector, route, handler, ...) | |
error(('incorrect http handler for %s %s: expecting return response object'): | ||
format(route.method, route.path), 0) | ||
end | ||
return { | ||
return labels_serializer.wrap({ | ||
path = route.path, | ||
method = route.method, | ||
status = (not ok and 500) or result.status or 200, | ||
} | ||
}) | ||
end, handler, ...) | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,86 @@ | ||||||
--- Default slow algorithm, polluted with sorting. | ||||||
--- It is used when nothing is known about `label_pairs`. | ||||||
local function default_labels_serializer(label_pairs) | ||||||
local parts = {} | ||||||
for k, v in pairs(label_pairs) do | ||||||
table.insert(parts, k .. '\t' .. v) | ||||||
end | ||||||
table.sort(parts) | ||||||
return table.concat(parts, '\t') | ||||||
end | ||||||
|
||||||
|
||||||
--- Prepares a serializer for label pairs with given keys. | ||||||
--- | ||||||
--- `make_key`, which is used during every metric-related operation, is not very efficient itself. | ||||||
--- To mitigate it, one could add his own serialization implementation. | ||||||
--- It is done via passing `__metrics_serialize` callback to the label pairs table. | ||||||
--- | ||||||
--- This function gives you ready-to-use serializer, so you don't have to create one yourself. | ||||||
--- | ||||||
--- BEWARE! If keys of the `label_pairs` somehow change between serialization turns, it would raise error mostlikely. | ||||||
--- We cover internal cases already, for example, "le" key is always added for the histograms. | ||||||
--- | ||||||
--- @class LabelsSerializer | ||||||
--- @field wrap function(label_pairs: table): table Wraps given `label_pairs` with an efficient serialization. | ||||||
--- @field serialize function(label_pairs: table): string Serialize given `label_pairs` into the key. | ||||||
--- Exposed so you can write your own serializers on top of it. | ||||||
--- | ||||||
--- @param labels_keys string[] Label keys for the further use. | ||||||
--- @return LabelsSerializer | ||||||
local function basic_labels_serializer(labels_keys) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming is confusing here: |
||||||
-- we always add keys that are needed for metrics' internals. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it is not internals -- it is histogram labeling approach (based on Prometheus design). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also have similar label -- |
||||||
local __labels_keys = { "le" } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also seems non-optimized: we will have a key that works only for a histogram. Maybe it's worth to have a different factories for counter/gauge, histogram and summary? |
||||||
-- used to protect label_pairs from altering with unexpected keys. | ||||||
local keys_index = { le = true } | ||||||
|
||||||
-- keep only unique labels | ||||||
for _, key in ipairs(labels_keys) do | ||||||
if not keys_index[key] then | ||||||
table.insert(__labels_keys, key) | ||||||
keys_index[key] = true | ||||||
end | ||||||
end | ||||||
table.sort(__labels_keys) | ||||||
|
||||||
local function serialize(label_pairs) | ||||||
local result = "" | ||||||
for _, label in ipairs(__labels_keys) do | ||||||
local value = label_pairs[label] | ||||||
if value ~= nil then | ||||||
if result ~= "" then | ||||||
result = result .. '\t' | ||||||
end | ||||||
result = result .. label .. '\t' .. value | ||||||
end | ||||||
end | ||||||
return result | ||||||
end | ||||||
|
||||||
local pairs_metatable = { | ||||||
__index = { | ||||||
__metrics_serialize = function(self) | ||||||
return serialize(self) | ||||||
end | ||||||
}, | ||||||
-- It protects pairs from being altered with unexpected labels. | ||||||
__newindex = function(table, key, value) | ||||||
if not keys_index[key] then | ||||||
error(('Label "%s" is unexpected'):format(key), 2) | ||||||
end | ||||||
rawset(table, key, value) | ||||||
end | ||||||
} | ||||||
|
||||||
return { | ||||||
wrap = function(label_pairs) | ||||||
return setmetatable(label_pairs, pairs_metatable) | ||||||
end, | ||||||
serialize = serialize | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
end | ||||||
|
||||||
return { | ||||||
default_labels_serializer = default_labels_serializer, | ||||||
basic_labels_serializer = basic_labels_serializer | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -269,3 +269,41 @@ g.test_deprecated_version = function(cg) | |
"use require%('metrics'%)._VERSION instead." | ||
t.assert_not_equals(cg.server:grep_log(warn), nil) | ||
end | ||
|
||
g.test_labels_serializer_consistent = function() | ||
local shared = require("metrics.collectors.shared") | ||
|
||
local label_pairs = { | ||
abc = 123, | ||
cba = "123", | ||
cda = 0, | ||
eda = -1, | ||
acb = 456 | ||
} | ||
local label_keys = {} | ||
for key, _ in pairs(label_pairs) do | ||
table.insert(label_keys, key) | ||
end | ||
|
||
local serializer = metrics.labels_serializer(label_keys) | ||
local actual = serializer.serialize(label_pairs) | ||
local wrapped = serializer.wrap(table.copy(label_pairs)) | ||
|
||
t.assert_equals(actual, shared.make_key(label_pairs)) | ||
t.assert_equals(actual, shared.make_key(wrapped)) | ||
t.assert_not_equals(wrapped.__metrics_serialize, nil) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like a unit test, so it's better to move it to somewhere else (maybe a new file). We also need to have an integration test with a full example:
It also would be nice ti write an example (at least in tests) for a custom serializer considering default metrics, especially since they are enabled by default in Tarantool 2.11.1 and newer. As a reference, you may refer to Telegraf labels setup here: https://www.tarantool.io/en/doc/latest/book/monitoring/grafana_dashboard/#prepare-a-monitoring-stack (code block starting with We should also do an integration test with global labels setup (AFAIU, one should cover it in serializer too). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, it seems that I have misunderstood the general approach: this PR introduces a serializer for each label pairs instead of replacing a serializer for each metrics. In that case, default metrics won't be covered. On the other hand, the question about global labels still needs to be answered. |
||
|
||
-- trying to set unexpected label. | ||
t.assert_error_msg_contains('Label "new_label" is unexpected', function() wrapped.new_label = "123456" end) | ||
|
||
-- check that builtin metrics work. | ||
local hist_serializer = metrics.labels_serializer(label_keys) | ||
local hist = metrics.histogram('hist', 'test histogram', {2}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, test with all collector types: counter, gauge, histogram and summary ( |
||
|
||
hist:observe(3, hist_serializer.wrap(table.copy(label_pairs))) | ||
local state = table.deepcopy(hist) | ||
hist:observe(3, label_pairs) | ||
|
||
t.assert_equals(hist.observations, state.observations) | ||
t.assert_equals(hist.label_pairs, state.label_pairs) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.