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

round durations to the second in events, errors and messages #1083

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmmorin
Copy link

@tmmorin tmmorin commented Feb 7, 2024

This is a small evolution so that messages like health check failed after 30.040865791s: timeout are made shorter and more readable, by rounding the duration to the nearest second and avoid useless decimals. The new messages will look like health check failed after 30s: timeout.

This is aimed to make logs, events and status conditions messages a bit more readable.

@tmmorin tmmorin force-pushed the nicer-duration-display branch 2 times, most recently from 7379adb to 167587d Compare February 12, 2024 17:11
@stefanprodan stefanprodan added the area/ux In pursuit of a delightful user experience label Feb 12, 2024
@tmmorin
Copy link
Author

tmmorin commented Feb 13, 2024

hello @stefanprodan
am I right to think that the CI check error is unrelated to my PR ?

@stefanprodan
Copy link
Member

@tmmorin can you rebase with upstream main and force push. The CI error should be fixed.

@tmmorin
Copy link
Author

tmmorin commented Apr 9, 2024

@tmmorin can you rebase with upstream main and force push. The CI error should be fixed.

done

@stefanprodan
Copy link
Member

Ok so tests are failing due to Health check passed in 0s. I think we should print the ms if the duration is under 1s

@tmmorin
Copy link
Author

tmmorin commented Apr 9, 2024

Ok so tests are failing due to Health check passed in 0s. I think we should print the ms if the duration is under 1s

ah, fair point indeed

@tmmorin tmmorin force-pushed the nicer-duration-display branch 2 times, most recently from 5ffd182 to dc082a2 Compare April 12, 2024 16:41
duration rounded:
- to the second for durations >= 1s
- to the ms for durations < 1s

Signed-off-by: Thomas Morin <[email protected]>
@tmmorin
Copy link
Author

tmmorin commented Apr 15, 2024

I made the suggested change, however this unit test is failing:

=== RUN   TestKustomizationReconciler_WaitConditions/emits_event_for_the_new_revision
=== NAME  TestKustomizationReconciler_WaitConditions
     kustomization_wait_test.go:263: 
         Expected
             <bool>: false
         to be true

I don't get why because, for sure, the logic for generating events is not changed by my PR.

@stefanprodan
Copy link
Member

for sure, the logic for generating events is not changed by my PR.

It is changing a lot, events are no longer unique and they get dropped by deduplication.

@tmmorin
Copy link
Author

tmmorin commented Apr 15, 2024

It is changing a lot, events are no longer unique and they get dropped by deduplication.

Ok, I see: implicitly the current code (at least the testing code) is relying on timing as a source of "unique event ids". This is unusual.

Should the way events are tested be changed ? (e.g. byt testing "new event" or "lastTimestamp is after last action")

Or should the side-effect of the new behavior behind this change be avoided ?

Including the metadata.generation in the messages is feasible for "Reconciliation finished in" : e.g "Reconciliation of generation %d finished in ...". But this idea wouldn't work for "Reconciliation failed after ....", nor for "Health check failed after..." or "Health passed in...".

@stefanprodan
Copy link
Member

stefanprodan commented Apr 15, 2024

It’s not the testing code the issue here but Kubernetes itself which doesn’t create a new event and drops duplicates, we’re been avoiding by having the full duration in the message so Kubernetes will not see it as a duplicate. Also having the generation with not fix anything as the same generation can reconcile new state from Git.

@stefanprodan
Copy link
Member

To fix the tests we probably have to look at the increment number and determine deduplication.

@tmmorin
Copy link
Author

tmmorin commented Apr 15, 2024

Yes, yes, I got it -- the test expects a new event and works only if the events escape deduplication by having pseudo-unique messages.

Also having the generation with not fix anything as the same generation can reconcile new state from Git.

Yes, indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ux In pursuit of a delightful user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants