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

[Redis] Add slow log metrics #1400

Merged
merged 1 commit into from Feb 27, 2015
Merged

[Redis] Add slow log metrics #1400

merged 1 commit into from Feb 27, 2015

Conversation

remh
Copy link
Contributor

@remh remh commented Feb 26, 2015

Supersedes #1330

Creates a histogram per redis command (HGET, LPUSH, etc) that is in the
slow query log.

Superseed #1330

Creates a histogram per redis command (HGET, LPUSH, etc) that is in the
slow query log.
@remh remh added this to the 5.3.0 milestone Feb 26, 2015
@remh remh added the 3 - Done label Feb 26, 2015
@remh remh changed the title Add redis slow log metrics [Redis] Add slow log metrics Feb 26, 2015
remh added a commit that referenced this pull request Feb 27, 2015
@remh remh merged commit 93f3d24 into master Feb 27, 2015

command_tag = 'command:{0}'.format(slowlog['command'].split()[0])
value = slowlog['duration']
self.histogram('redis.slowlog.micros', value, tags=tags + [command_tag])
Copy link
Member

Choose a reason for hiding this comment

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

do you think it makes sense to convert to something other than microseconds (like milliseconds or seconds) since we don't really use that for any other metrics? If not, we might want to spell out "microseconds" or something. Maybe just me but I didn't know what "micros" meant without looking at the docs for the redis slowlog.

Copy link
Member

Choose a reason for hiding this comment

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

on second thought, a conversion might be a bad idea because it would make the axis all wonky

Copy link
Contributor

Choose a reason for hiding this comment

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

I think micros is good enough - or even in_micros which us better, IMHO, than μs which would likely become us anyway. We have used micros for a long time with cassandra metrics too.

@conorbranagan
Copy link
Member

Cool metrics, interested to see what sort of data we can get from this on prod. Do you know if the commands in transactions will show up or will it just show up as exec?

if max_slow_entries > DEFAULT_MAX_SLOW_ENTRIES:
self.warning("Redis {0} is higher than {1}. Defaulting to {1}."\
"If you need a higher value, please set {0} in your check config"\
.format(max_slow_entries, DEFAULT_MAX_SLOW_ENTRIES))
Copy link
Member

Choose a reason for hiding this comment

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

This will result in having a string like

Redis 256 is higher than 128. Defaulting to 128. If you need a higher value please set 256 in your check config

Which is not super understandable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed - the wording here should help the user understand what it is we're trying to tell them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arf i reused the same variable name, max_slow_entries was supposed to be "slowlog-max-len" good catch!

@LeoCavaille
Copy link
Member

Yep @conorbranagan I think only individual commands go in the slow log, even if they're grouped in a transaction.


# Max number of entries to fetch from the sloq query log
# By default, the check wil read this value from the redis config
# But if it's too high, it will default to 128
Copy link
Contributor

Choose a reason for hiding this comment

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

"too high" should be spelled out, why is too high too high? "Higher than 128, due to potential increased latency to retrieve more than 128 slowlog entries every 15 seconds"...

@remh
Copy link
Contributor Author

remh commented Feb 27, 2015

Thanks for the comments, i took care of them 6f52aee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants