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.

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

fix scrapping for redis < 4.0 #860

Merged

Conversation

jcaspes
Copy link
Contributor

@jcaspes jcaspes commented Dec 14, 2023

This fix avoid an exception to bee thrown so scrapping fail for redis version < 4.0
Metrics will so only contains keys_group_count and not memory_usage
Tested on docker redis 3.2.4: (docker pull r900/redis:3.2.4)

This fix avoid an exception to bee thrown so scrapping fail for redis version < 4.0
Metrics will so only contains keys_group_count and not memory_usage
tested on docker redis 3.2.4: (docker pull r900/redis:3.2.4)
@@ -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

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!

Was trying to index a number in case of success
Better to check type to check if MEMORY USAGE command succeed, so use the result (key memory usage bytes)
@coveralls
Copy link

Pull Request Test Coverage Report for Build 229

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 92.021%

Totals Coverage Status
Change from base Build 227: 0.01%
Covered Lines: 1949
Relevant Lines: 2118

💛 - Coveralls

local reply = redis.pcall("MEMORY", "USAGE", key)
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

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!

@oliver006 oliver006 merged commit 4a6b542 into oliver006:master Dec 17, 2023
5 checks passed
@jcaspes jcaspes deleted the fix_key_group_scrapping_old_redis_versions branch December 17, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants