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

Do not update activity on failed requests #292

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

minrk
Copy link
Member

@minrk minrk commented Mar 5, 2021

Up to now, we mark activity for any attempt to talk to an endpoint, by updating the activity timestamp when any request is initiated.

This means that an endpoint that isn't even running anymore (503 errors) will register activity if someone is trying to talk to it.

This moves the activity update to the response finish event, which is when the request completes. This lets us take the status code into account, and only consider 'successful' (status < 300) requests to be activity.

Consequences:

  • a server that is unavailable (always status 503) will not appear 'active'
  • spurious requests (e.g. lots of 404s because the endpoint isn't what is expected) no longer count as activity
  • borderline: redirects (3XX) are not counted either, which I think is right because we can assume a redirect will result in a second request to the same server that will be either successful or a failure, and thus accounted for on the retry. If it is not retried, or if the 'right' destination is another service, this shouldn't be considered 'activity'.
  • Activity is registered slightly later - at the end of requests rather than the beginning
  • Opening a websocket will not register as activity until a first message is sent or received

The last two points mean that there can technically be a delay (usually at most 30 seconds due to various socket timeouts) between when activity is initiated and when it is registered. However, the same change also means that the 'last' activity timeout is also more accurate - more closely aligning with the end of the transaction than the beginning.

This additionally tweaks logging to omit url params, which are not relevant to the proxy.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

This LGTM nice, thanks for excellent PR summary!

@consideRatio consideRatio merged commit 2417b11 into jupyterhub:master Mar 5, 2021
@minrk
Copy link
Member Author

minrk commented Mar 5, 2021

Thanks for the quick merge! #293 adds some test coverage for the new behavior

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

Successfully merging this pull request may close these issues.

2 participants