Skip to content

Commit

Permalink
collect redis commands as a per second metric instead of per flush in…
Browse files Browse the repository at this point in the history 
…terval (fixes #697)
  • Loading branch information
Carlo Cabanilla committed Oct 24, 2013
1 parent 71df2cb commit 26f6415
Showing 1 changed file with 2 additions and 7 deletions.
9 changes: 2 additions & 7 deletions checks.d/redisdb.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ class Redis(AgentCheck):

def __init__(self, name, init_config, agentConfig):
AgentCheck.__init__(self, name, init_config, agentConfig)
self.previous_total_commands = {}
self.connections = {}

def get_library_versions(self):
Expand Down Expand Up @@ -162,12 +161,8 @@ def _check_db(self, instance, custom_tags=None):
[self.rate (self.RATE_KEYS[k], info[k], tags=tags) for k in self.RATE_KEYS if k in info]

# Save the number of commands.
total_commands = info['total_commands_processed'] - 1
tuple_tags = tuple(tags)
if tuple_tags in self.previous_total_commands:
count = total_commands - self.previous_total_commands[tuple_tags]
self.gauge('redis.net.commands', count, tags=tags)
self.previous_total_commands[tuple_tags] = total_commands
self.rate('redis.net.commands', info['total_commands_processed'],
tags=tags)

def check(self, instance):
try:
Expand Down

5 comments on commit 26f6415

@clutchski
Copy link
Contributor

Choose a reason for hiding this comment

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

should we rename this to redis.net.commands_per_sec so there is no confusion?

@clofresh
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should make a new metric, the previous implementation was just broken, and we shouldn't have metrics that aren't normalized per second. I think we can just put a note in the changelog saying that your redis.net.commands metric will change if you upgrade

@elubow
Copy link

@elubow elubow commented on 26f6415 Oct 26, 2013

Choose a reason for hiding this comment

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

tl;dr I think renaming is a bad idea but there should be some level of communication to customers (like me) that the metric is changing to be correct.

I think the issue here isn't just that the change is happening if you upgrade, but if you upgrade your metrics will now be correct. My redis.net.commands graph was reading 600k ops/sec on some Redis instances (which is clearly unrealistic). So if I were to divide that by 15 to get the correct values, my graph would change drastically and I think it's important to note why it has changed so drastically (for customers who look at their Redis graphs that didn't know that this was an issue). I only know because I reported it. The other folks on my team were going along thinking we were pushing Redis harder than it's been pushed before (and I assume others will have the same thoughts).

@clutchski
Copy link
Contributor

Choose a reason for hiding this comment

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

@elubow Thanks for chiming in. Maybe we'll let folks opt-in to an 'agent update' e-mail or post an event in everybody's stream with a link to the changelog. That should give folks a heads up that things are evolving. I'll bring it up with the team on Monday.

@remh
Copy link

@remh remh commented on 26f6415 Dec 5, 2013

Choose a reason for hiding this comment

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

Communication will be handled with 4.0.0 release communication.

Please sign in to comment.