-
Notifications
You must be signed in to change notification settings - Fork 147
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
feat: Allow Skipping of Activity Reporting #473
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
Thanks for the contribution @sdmichelini ! This seems like it would be useful if you want the culler to ignore a proxied app's activity, and I think your change will have the effect you intend. Could you also please add an entry in Lastly, it'd also be great if you could add a test case under |
@ryanlovett Thanks for the prompt review! I've added documentation for the new parameter in the suggested location. As for the tests is there a config-test pattern to follow that you would recommend? I did a check over the current tests and don't see ones for config where I could plug in other than confirm basic launching of the HTTP server. |
@sdmichelini I think you'd need to create a server to proxy that generates "spurious" activity, such as the kind that you want to mask via this PR. Then you'd need to add an entry to I believe the hub uses /api/status to check for activity. I would imagine This is all easier for me to say than do, so let me know if you have any questions. |
Hmm ok, looks more convoluted than I was intending for as I'm not all to familiar with the codebase - what I did to test this was to manually check the hubs activity time when I had a browser session open and my pod was culled. |
No worries. Are you able to share info about what app you were proxying? (in the event that we can simulate its behavior) |
Yes - https://github.com/coder/code-server - is the app I am proxying. |
@ryanlovett any objections to adding this? would be useful to have this feature. |
Looking forward for this feature as well. |
@sdmichelini No objections. Let me add another reviewer... |
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.
Thank you for the PR @sdmichelini and thank you for tagging me in, @ryanlovett.
This looks good to me, but I'd like to request the name of this be changed. The activity tracking is really a part of jupyter_server, and can be used by any application (not just JupyterHub, although it's the most common user).
In jupyter_server (and in JupyterHub too I think) it's called 'last_activity'.
So I suggest the following course of action:
- Change the name of the property to
update_last_activity
- When set to
True
, it will retain current default behavior. When set toFalse
, it will no longer updatelast_activity
. This is an inverse of the current config setting. The default will beTrue
(inversion of this PR's default again)
I'm happy to merge this once this change is done.
Thanks!
Allows proxy to be configured if it reports back to JupyterHub. In some cases the proxy will always detect background activity which means that the pod will never get culled. In this case you must disable recording and use an alternative source of activity(outside of this MR) Closes jupyterhub#471
Add documentation for new property.
Update name and verbiage.
@yuvipanda Thank you for taking a look! I have updated the pull-request based on your feedback! |
Thanks a lot, @sdmichelini! Looking forward to more PRs from you :) |
Allows proxy to be configured if it reports back to JupyterHub. In some cases the proxy will always detect background activity which means that the pod will never get culled. In this case you must disable recording and use an alternative source of activity(outside of this MR).
Closes #471