-
Notifications
You must be signed in to change notification settings - Fork 212
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
added timer to trigger send if no incoming transactions trigger it #68
Conversation
4f79e4c
to
9070b92
Compare
a148d30
to
9b41ca4
Compare
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.
@beniwohli I tried to walk through possible race conditions that could happen. It looks like you are pretty good covered with the usage of with thread.cond
so from the understanding I have this looks good.
self.interval = interval | ||
self.callback = callback | ||
self.task = asyncio.ensure_future(self._job()) | ||
self._done = False |
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.
What for are you using this self._done
information?
@@ -456,6 +462,8 @@ def set_transaction_extra_data(self, data, _key=None): | |||
|
|||
def close(self): | |||
self._traces_collect() | |||
if self._send_timer: |
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.
Is it possible that a new transaction is started during the time that close
is processing? If so, it looks like it could happen that the end_transaction
could trigger a new _start_send_timer
. This is probably an edge case that shouldn't be too bad though, because I assume when close()
is called the whole application is going to be restarted..
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.
exactly. Outside of test scenarios, .close()
is only called by application shutdown logic. At this point, the application shouldn't accept any new transactions.
This should alleviate a point of confusion when users expected that we send data every x seconds. Instead, we only sent data every x seconds if there were incoming transactions. If no transactions were happening, no data was sent (including existing transactions on the queue). This PR aligns the behaviour with user expectations. The trade-offs are: * slightly more resource consumption due to the timer thread * collection can either happen on the main thread, or on the timer thread, depending on whether the timer or an incoming transaction triggers collection.
87298ba
to
478a205
Compare
This should alleviate a point of confusion when users expected that we
send data every x seconds. Instead, we only sent data every x seconds
if there were incoming transactions. If no transactions were happening,
no data was sent (including existing transactions on the queue).
This PR aligns the behaviour with user expectations. The trade-offs are:
thread, depending on whether the timer or an incoming transaction
triggers collection.