-
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
Create message rate store #60
Create message rate store #60
Conversation
Completely without context for what the details of this are :) but if you're using Rabbitmq for the queue it does ship with an API that publishes rate data: https://cdn.rawgit.com/rabbitmq/rabbitmq-management/rabbitmq_v3_5_6/priv/www/api/index.html |
@imsickofmaps We're planning to include queue sizes and message rates from
|
@hodgestar awesome, carry on 😀 |
@hodgestar Ready for review. |
Note: bucket_size should be kept constant for each channel_id and label | ||
combination. Changing bucket sizes results in undefined behaviour.''' | ||
key = self._get_current_key(channel_id, label, bucket_size) | ||
self.ttl = bucket_size * 2 |
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.
Should bucket_size
not be passed to the MessageRateStore
constructor? That would avoid having to pass it in to the individual methods and prevent people from passing in different values?
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.
If we pass the bucket size into the constructor, we can also set the ttl there.
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.
I originally wanted to inherit as much as possible from the BaseStore
, but I see now that it is hurting us more than helping us.
The reason I wanted bucket_size
to be passed per function is that in the API when we're fetching the msgs/s, inbound
and outbound
could be configured to have different bucket sizes. This reason is why we had channel_id
being passed to every function for the other stores, instead of setting the channel id in the constructor.
The reason that ttl
is being set here, is because we want to set the ttl when we increment, but we don't want to set the ttl again when we fetch the value (there we set ttl
to None
, so that we don't call EXPIRE
.
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.
Maybe we need a ttl
flag that can override self.ttl
? Or a reset_ttl=False
flag? (for certain redis ops).
Left some questions about bucket size and TTL setting, but otherwise looks good. |
@hodgestar I've added a |
@@ -19,32 +20,44 @@ def __init__(self, redis, ttl=None): | |||
|
|||
@inlineCallbacks | |||
def _redis_op(self, func, id, *args, **kwargs): | |||
ttl = kwargs.pop('ttl') | |||
if ttl is None: | |||
ttl = self.ttl |
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.
If self.ttl = 5
, setting ttl = None
in the kwargs won't override the 5
? Just checking whether that is intentional?
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.
Yes this is intentional. None
means to use the default (and is the default for all of the methods) that is set when you create the class. I'm not sure if it's possible to make this clearer, maybe if the argument doesn't exist, it should use the default, and if it does exist, it uses that value? It would make things a bit more complicated though. Thoughts?
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.
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.
I think we can do that, yes. Looks good.
@hodgestar Changes made, ready for another review. |
👍 |
…rate-store Create message rate store
Create a store that can get called whenever the counter should be incremented, and be able to get the current msgs/s, with configurable bucket sizes.