Add hook for formatting Kubernetes Event messages#910
Add hook for formatting Kubernetes Event messages#910agoose77 wants to merge 63 commits intojupyterhub:mainfrom
Conversation
for more information, see https://pre-commit.ci
Whatever we decide we need to be consistent in future. If we add GCP specific code we need to accept code for other clouds, including from third parties who use platforms that we can't test ourselves. |
dd73955 to
cb2f0a3
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
I enjoy this feature ❤️ I like that the format_event_hook was easy to configure for basic formatting. It took me a while to understand what was going on with the default formatting, but I think there will alway be room for improvements there.
In general, my main comment is to complete the test suite to regenerate how the sample-events were created through the message.py, since I think that represents the bulk of the work in this PR. The default formatting may undergo further development, but like you say I think we want to add in some basic regression testing and update that if needed in future.
In answer to your questions:
Is Kubespawner making too many assumptions if we bake-in the expectation of Bootstrap?
We know Bootstrap ships with JupyterHub, so I think this is a safe assumption for now. I don't think we need to worry about this for BinderHub?
Could we consider adding a start-time timestamp so that times can simply be given as "minutes since spawn" rather than UTC times?
I think this is a nice-to-have. Most users hopefully shouldn't have to dwell on the spawn progress screen, but if they do, then they will likely screenshot their spawn failure to send to an admin. Keeping the timestamps consistent with server side logs with raw k8s events should be useful for sysadmins for troubleshooting.
Should I rework the built-in formatter to generate HTML at every stage — would it be useful to have e.g. image names/tags, and node names in button-tags?
At this stage, I would prefer not to have the default formatter be too flashy.
Is there motivation to move the default message format into its own configurable, rather than requiring users to create their own hook?
Yes, out of all of these questions I think this would be one to focus on. Most people will probably want to configure an extension to the default formatter.
I am okay with that – I don't think we need to assume full responsibility for testing code on platforms we don't have access to, but we should ensure contributors who would like this functionality to include full test suites for that. I think the scope of changes in this PR are pretty cosmetic, so there doesn't seem to be huge scope for someone to introduce anything too crazy on a third party platform. There is a small question about how to structure this as the corpus of messages to reformat scales, but I think we can cross that bridge when we get to it? |
|
@yuvipanda rather than moving just the business logic of event formatting to the new module created in this PR, I opted to move it into a new module under a new Configurable. This helps better isolate the responsibility, makes testing easier, and also improves extensibility. |
for more information, see https://pre-commit.ci
| assert "progress" in progress | ||
| assert isinstance(progress["progress"], int) | ||
| assert "message" in progress | ||
| assert isinstance(progress["message"], str) | ||
| messages.append(progress["message"]) |
There was a problem hiding this comment.
My LSP changes these. It passes pre-commit, so I assume OK to leave?
| or "", | ||
| "message": event.get("message") or "", | ||
| "reason": event.get("reason") or "", | ||
| "type": event["type"], |
There was a problem hiding this comment.
Added the type field, as it's harmless and useful!
jnywong
left a comment
There was a problem hiding this comment.
Nice, a significant improvement over the last time I reviewed this. It makes sense to make all of the matching rulesets configurable rather than essentially hard coding them into the library itself as before. Good job!
I have minor style suggestions and comments, but I think this is gtg once addressed.
Co-authored-by: Jenny Wong <jnywong.pro@gmail.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
I tested the scenario where I triggered a
Could you take a close look at this? I am not sure why this rule would match the |
|
@jnywong nice catch! This PR adds a default formatter that was not being returned properly. I've lifted it earlier into the transforms to avoid needing to manually handle the exception case. |



TL;DR
Note
No LLMs were used in the authoring of this PR.
2i2c is currently working on a user-story to improve the kubespawner progress messages, as part of an initiative to improve the spawn-progress page.
This PR does several things:
decorate_progress_messagethat overrides the pretty-printing of event messages.kubespawner.eventsmodule for richer built-in formatting of log messages.kubespawner.events.RuleEventFormatterand other types for defining event formatting rules.See the before and after:
Before

After

Goal
The goal is modest: to improve the human readability of spawn messages, and to allow further customisation.
Example Decoration Hook
Basic hook
Use the rules to define custom renderers
Design Details
UI
%Y-%m-%DTHH:MM:SSZto keep fixed widthConstraints
pyproject.toml, meaning nomatch,:=, orremoveprefix.Questions