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

fix scrapping for redis < 4.0 #860

Merged
merged 2 commits into from
Dec 17, 2023
Merged

fix scrapping for redis < 4.0 #860

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b78512c
fix scrapping for redis < 4.0
jcaspes Dec 14, 2023
1bc1b45
Fix bad error on type
jcaspes Dec 15, 2023
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
5 changes: 4 additions & 1 deletion exporter/key_groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,10 @@ for i=3,#ARGV do
end
end
for i,key in ipairs(batch[2]) do
usage = redis.call("MEMORY", "USAGE", key)
local reply = redis.pcall("MEMORY", "USAGE", key)
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seem to work, see the tests:

=== RUN   TestKeyGroupMetrics/synchronous_with_unclassified_keys

time="2023-12-14T11:01:58Z" level=error msg="ERR Error running script (call to f_2111fe4ef69857c0b23529937bb692b02c475a9b): @user_script:20: user_script:20: attempt to index local 'reply' (a number value)"

    key_groups_test.go:159: Key group count metrics are not expected:

         Expected: map[string]int{"key_exp":5, "key_paul":1, "key_ringo":1, "unclassified":6}

        Actual: map[string]int{}

    key_groups_test.go:167: Key group memory usage metrics are not expected:

         Expected: map[string]bool{"key_exp":true, "key_paul":true, "key_ringo":true, "unclassified":true}

        Actual: map[string]bool{}

    key_groups_test.go:171: Unexpected number of distinct key groups, expected: 4, actual: 0

=== RUN   TestKeyGroupMetrics/synchronous_with_overflow_keys

time="2023-12-14T11:01:58Z" level=error msg="ERR Error running script (call to f_2111fe4ef69857c0b23529937bb692b02c475a9b): @user_script:20: user_script:20: attempt to index local 'reply' (a number value)"

    key_groups_test.go:159: Key group count metrics are not expected:

         Expected: map[string]int{"overflow":12, "test-stream":1}

        Actual: map[string]int{}

    key_groups_test.go:167: Key group memory usage metrics are not expected:

         Expected: map[string]bool{"overflow":true, "test-stream":true}

        Actual: map[string]bool{}

    key_groups_test.go:171: Unexpected number of distinct key groups, expected: 13, actual: 0

--- FAIL: TestKeyGroupMetrics (0.11s)

Copy link
Contributor Author

@jcaspes jcaspes Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oups, my bads i haven't tested on a version that was supporting MEMORY command.
It should be better now by checking the returned type
1bc1b45

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good now, thanks!

if type(reply) == "number" then
usage = reply;
end
Copy link
Owner

Choose a reason for hiding this comment

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

Question, should you exit if you don't get a value for usage?
How will that get handled down in line 193/195 ?

Copy link
Contributor Author

@jcaspes jcaspes Dec 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hello,

local usage variable is initialized to 0 so no problem on line 193/195, is value will stay to 0 if MEMORY do not work

The idea is to obtain at less key_groups count on redis version prior to 4.0, groups will always contain 0 for memory usage but the correct count.

A possibility to make it better should be to not generate memory usage metrics when not available... but it's more complex and need more code modifications (but it is possible to do it ;-) ).

The current workaround make possible to use a limited key_groups scrapping that i think is better than the today error that generate nothing for key_group for legacy redis.

Copy link
Owner

Choose a reason for hiding this comment

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

That makes sense, thanks for the explanation!

group = nil
for i=3,#ARGV do
key_match_result = {string.find(key, ARGV[i])}
Expand Down
Loading