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

feat: Allow Skipping of Activity Reporting #473

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sdmichelini
Copy link

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

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
Copy link

welcome bot commented Apr 15, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@ryanlovett
Copy link
Collaborator

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 doc/source/server-process.md?

Lastly, it'd also be great if you could add a test case under tests/.

Add documentation for new property.
@sdmichelini
Copy link
Author

@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.

@ryanlovett
Copy link
Collaborator

@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. resources/httpinfo.py is a simple server, but for this test implementation you'd need to write one that is more active. That might be the most difficult part.

Then you'd need to add an entry to jupyter_server_config.py that sets the new configuration option and proxies the busy test server, and write a test function in test_proxies.py that queries the proxy for activity. It would need to confirm that there is no activity even when you might expect there to be some.

I believe the hub uses /api/status to check for activity. I would imagine last_activity is not updated frequently, or at all, when you are not proxying your real world busy app, but is updated when that app is running. If so then your test would confirm that, with your new configuration option set, last_activity is not updated even when the busy test server is running.

This is all easier for me to say than do, so let me know if you have any questions.

@sdmichelini
Copy link
Author

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.

@ryanlovett
Copy link
Collaborator

No worries. Are you able to share info about what app you were proxying? (in the event that we can simulate its behavior)

@sdmichelini
Copy link
Author

Yes - https://github.com/coder/code-server - is the app I am proxying.

@sdmichelini
Copy link
Author

@ryanlovett any objections to adding this? would be useful to have this feature.

@deser
Copy link

deser commented Jun 14, 2024

Looking forward for this feature as well.

@ryanlovett
Copy link
Collaborator

@sdmichelini No objections. Let me add another reviewer...

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

Successfully merging this pull request may close these issues.

Ability to Configure Activity Reporting
3 participants