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

Invalid Date showing with salt 3005.1 #485

Merged
merged 7 commits into from
Oct 26, 2022
Merged

Invalid Date showing with salt 3005.1 #485

merged 7 commits into from
Oct 26, 2022

Conversation

erwindon
Copy link
Owner

Describe the bug
Salt-master 3005.1 on Ubuntu 20.04, UI shows "Invalid.731210 Date" as and example for all the start times.

Here is the output from the master's console for salt '*' state.apply

octopi:
----------
          ID: timezone_packages
    Function: pkg.installed
        Name: tzdata
      Result: True
     Comment: All specified packages are already installed
     Started: 12:09:31.983339
    Duration: 274.309 ms
     Changes:
----------
          ID: dbus_for_timezone
    Function: pkg.installed
        Name: dbus
      Result: True
     Comment: All specified packages are already installed
     Started: 12:09:32.258763
    Duration: 88.489 ms
     Changes:
----------
          ID: dbus_for_timezone
    Function: service.running
        Name: dbus
      Result: True
     Comment: The service dbus is already running
     Started: 12:09:32.356435
    Duration: 189.594 ms
     Changes:
----------
          ID: timezone_setting
    Function: timezone.system
        Name: America/Toronto
      Result: True
     Comment: Timezone America/Toronto already set, UTC already set to America/Toronto
     Started: 12:09:32.554879
    Duration: 214.925 ms
     Changes:

Summary for octopi
------------
Succeeded: 4
Failed:    0
------------
Total states run:     4
Total run time: 767.317 ms

Note the timestamp formats for the started time.

Screenshots

Screen Shot 2022-10-20 at 12 16 44

@erwindon
Copy link
Owner

converted to PR

@erwindon
Copy link
Owner

this can be a problem in the original data or even in the settings of the browser.
I've added several debug-statements in the function that handles the date+time formatting in the edition from this git branch.
can you give it a try and let me know what the output is?
(the output is visible in the web-browser "console")

@erwindon
Copy link
Owner

p.s. the output in the original description is from the output of a salt command, this is not the metadata from the job itself. that does not change anything for the problem though.

@mkbrown
Copy link
Author

mkbrown commented Oct 20, 2022

Here's a couple of cycles worth of data. Hope this is what you are looking for:

[Log] utcDT1 – "Invalid Date" (Output.js, line 243)
[Log] utcDT2 – "Invalid Date" (Output.js, line 245)
[Log] localDT1 – "Invalid Date" (Output.js, line 253)
[Log] localTZ1 – "Date" (Output.js, line 255)
[Log] localDT2 – "Invalid" (Output.js, line 257)
[Log] ret1 – "Invalid.240089 Date" (Output.js, line 284)
[Log] utcDT1 – "Invalid Date" (Output.js, line 288)
[Log] utcDT2 – "Invalid Date" (Output.js, line 290)
[Log] localDT1 – "Invalid Date" (Output.js, line 292)
[Log] localDT2 – "Invalid Date" (Output.js, line 294)

[Log] pDtStr1 – "2022, Oct 20 02:30:56.557406" (Output.js, line 165)
[Log] pDtStr4 – "2022, Oct 20 02:30:56" (Output.js, line 222)
[Log] utcDT1 – "Invalid Date" (Output.js, line 243)
[Log] utcDT2 – "Invalid Date" (Output.js, line 245)
[Log] localDT1 – "Invalid Date" (Output.js, line 253)
[Log] localTZ1 – "Date" (Output.js, line 255)
[Log] localDT2 – "Invalid" (Output.js, line 257)
[Log] ret1 – "Invalid.557406 Date" (Output.js, line 284)
[Log] utcDT1 – "Invalid Date" (Output.js, line 288)
[Log] utcDT2 – "Invalid Date" (Output.js, line 290)
[Log] localDT1 – "Invalid Date" (Output.js, line 292)
[Log] localDT2 – "Invalid Date" (Output.js, line 294)

[Log] pDtStr1 – "2022, Oct 20 02:29:32.411423" (Output.js, line 165)
[Log] pDtStr4 – "2022, Oct 20 02:29:32" (Output.js, line 222)

(edit by @erwindon to show the groups)

@mkbrown
Copy link
Author

mkbrown commented Oct 20, 2022

image

@mkbrown
Copy link
Author

mkbrown commented Oct 20, 2022

Looks like it might be a Safari issue, below is a partial screenshot from Edge on Mac which apparently works.

image

@erwindon
Copy link
Owner

I've added 2 blank lines in the output that you provided to show the logging of each invocation of the custom function dateTimeStr.
the input looks good: "2022, Oct 20 02:30:56.557406", so that is not the cause.
but then one of the functions toLocaleTimeString or toLocaleString is called which does not give a proper result. SaltGUI can be improved to handle that better.

the Edge-on-Mac looks ok, except that SaltGUI assumes that the string ends with the seconds when it appends the milliseconds part. I can improve that separately.

@erwindon erwindon assigned erwindon and unassigned mkbrown Oct 20, 2022
@erwindon
Copy link
Owner

TASKS:

  1. provide a sensible (possible lesser) alternative when one of functions toLocaleTimeString or toLocaleString fails to return a proper result; and
  2. provide a fix for the presentation of milliseconds in 12h clock systems.

@erwindon
Copy link
Owner

erwindon commented Oct 21, 2022

step 1: I simulated the fact that functions toLocaleTimeString and toLocaleString return the text Invalid Date. The output from the the log-statements now matches exactly with the situation in your browser.

step 2:
SaltGUI tries to show a tooltip with both of local-time and utc-time in full detail on every date/time field.
when this issue applies to the text in such tooltip, SaltGUI will not show that tooltip.

step 3:
when this applies to a normal text field, we'll use the default date/time formatting.
the setting saltgui_datetime_representation will then be ignored.

@erwindon erwindon force-pushed the datetime branch 2 times, most recently from d66df16 to b2ab67f Compare October 21, 2022 21:28
@erwindon
Copy link
Owner

also fixed the placement of the milliseconds part to before the timezone+ampm indicators

@erwindon
Copy link
Owner

can you please give it a try?
preferably on both systems (old/new)
please look at normal dates, short time (on Events screen) and tooltip (on any datetime field)

@erwindon erwindon assigned mkbrown and unassigned erwindon Oct 21, 2022
@mkbrown
Copy link
Author

mkbrown commented Oct 24, 2022

OK, so testing on a few browsers for comparison:

Edge Version 106.0.1370.47 (Official build) (arm64) on Mac 12.6 (21G115) M1 Processor.

image

Safari Version 16.0 (17614.1.25.9.10, 17614) on Mac (same Mac). Invalid dates.

image

Firefox on Windows Server 2019 via RDP session. Looks good.
image

Safari on iPad OS 16.0 (invalid dates)

image

@erwindon
Copy link
Owner

@mkbrown
I've (re)added a bit more debug logging.
can you please retry on one of the Safari instances? (after a new git pull of course)
and then please post the console output.
should be only a few lines per datetime this time
(I hate situations that I cannot replay myself...)

@mkbrown
Copy link
Author

mkbrown commented Oct 24, 2022

Here are the problem sections (they repeat)

[Log] dateObj – Invalid Date (Output.js, line 238)
[Log] utcDT2a – "Invalid Date" (Output.js, line 249)
[Log] utcDT2b – "Invalid Date" (Output.js, line 252)
[Log] localDT2a – "Invalid Date" (Output.js, line 267)
[Log] localDT2b – "Invalid Date" (Output.js, line 270)
[Log] dateObj – Invalid Date (Output.js, line 238)
[Log] utcDT2a – "Invalid Date" (Output.js, line 249)
[Log] utcDT2b – "Invalid Date" (Output.js, line 252)
[Log] localDT2a – "Invalid Date" (Output.js, line 267)
[Log] localDT2b – "Invalid Date" (Output.js, line 270)

@erwindon
Copy link
Owner

ah, it was already invalid at that point...
I've added a few more loglines, can you repeat?
the first logline in the sequence now has the marker "BEGIN" for easier grouping, but still having one group for investigation is sufficient

@mkbrown
Copy link
Author

mkbrown commented Oct 24, 2022

Here's the new debug info:

[Log] BEGIN pDtStr1 – "2022, Oct 24 20:05:21.939900" (Output.js, line 169)
[Log] pDtStr4 – "2022, Oct 24 20:05:21" (Output.js, line 226)
[Log] milliSecondsSinceEpoch – NaN (Output.js, line 239)
[Log] dateObj – Invalid Date (Output.js, line 241)
[Log] utcDT2a – "Invalid Date" (Output.js, line 254)
[Log] utcDT2b – "Invalid Date" (Output.js, line 257)
[Log] localDT2a – "Invalid Date" (Output.js, line 272)
[Log] localDT2b – "Invalid Date" (Output.js, line 275)

@erwindon erwindon assigned erwindon and unassigned mkbrown Oct 24, 2022
@erwindon
Copy link
Owner

in Safari, the function Date.parse is less flexible than for other browsers it seems.
but, admitted, it is still within the specification for it.
see also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse

@erwindon
Copy link
Owner

I simulated the part where milliSecondsSinceEpoch turned into NaN.
Based on that, a fallback is provided.
This time no new debugging statements, but I left the previous ones, just in case.
Can you please test once more?
preferably all setups that you used before, and looking at the full datetime representation (e.g. for a job) and the shorter time representation (e.g. for an event).

@mkbrown
Copy link
Author

mkbrown commented Oct 25, 2022

Looking good at this point! Times are showing as UTC on Mac though.

Safari on Mac

image

image

image

Edge on Mac. Times show as EDT (local timezone)

image

image

image

Firefox on Windows Server

image

image

image

Safari on iPad OS 16.0

image

Scrolled the viewport over to show the jobs screen with timestamps. It renders larger than the default view.

image

Similarly scrolled over...

image

Thanks again for the assistance with this!

@erwindon
Copy link
Owner

erwindon commented Oct 25, 2022

Times are showing as UTC on Mac though.

SaltGUI is showing the original datetime-string as it got from the salt-api here.
The additional logic in SaltGUI converts that into localtime; SaltGUI allows the representation to be chosen (always utc, always local, local+utc or utc+local); and SaltGUI adds a tooltip that just shows localtime+utc. but if that initial conversion cannot be done, then all these enhancements cannot work.
Is there anything that should still be updated on the date+time handling?
When all is ok, I will remove the debug-code and merge.

Please open a separate issue for any other problems. It is my policy not to discuss them in the context of another issue.

@erwindon erwindon assigned mkbrown and unassigned erwindon Oct 25, 2022
@mkbrown
Copy link
Author

mkbrown commented Oct 25, 2022

Started playing with some salt code, and noticed that it seems to be a fan of the Prince tune, 1999

Safari on Mac

image

Edge on Mac is showing some other oddities in date conversions. Note

image

Firefox on Windows Server is showing the same

image

target systemtime is set as such:

# timedatectl
Local time: Tue 2022-10-25 08:54:26 EDT
Universal time: Tue 2022-10-25 12:54:26 UTC
RTC time: Tue 2022-10-25 12:54:27
Time zone: America/Toronto (EDT, -0400)
System clock synchronized: yes
NTP service: active
RTC in local TZ: no

@erwindon
Copy link
Owner

that means party!

the date is actually 9-9-99
SalltGUI tries to display only the time here.
The debug logging should still be active.
Can you copy the debug output for the 1999-case here?

@mkbrown
Copy link
Author

mkbrown commented Oct 25, 2022

1999 case logs:

[Log] BEGIN pDtStr1 – "2022, Oct 25 12:46:50.754832" (Output.js, line 169)
[Log] pDtStr4 – "2022, Oct 25 12:46:50" (Output.js, line 226)
[Log] milliSecondsSinceEpoch – NaN (Output.js, line 239)
[Log] dateObj – Invalid Date  (Output.js, line 241)
[Log] utcDT2a – "Invalid Date" (Output.js, line 257)
[Log] utcDT2b – "Invalid Date" (Output.js, line 260)
[Log] localDT2a – "Invalid Date" (Output.js, line 281)
[Log] localDT2b – "Invalid Date" (Output.js, line 284)
[Log] BEGIN pDtStr1 – "1999, Sep 9 08:46:52.042499" (Output.js, line 169)
[Log] pDtStr4 – "1999, Sep 9 08:46:52" (Output.js, line 226)
[Log] milliSecondsSinceEpoch – NaN (Output.js, line 239)
[Log] dateObj – Invalid Date  (Output.js, line 241)
[Log] utcDT1a – "Invalid Date" (Output.js, line 246)
[Log] utcDT1b – "Invalid Date" (Output.js, line 250)
[Log] localDT1a – "Invalid Date" (Output.js, line 271)
[Log] localDT1b – "Invalid Date" (Output.js, line 274)
[Log] BEGIN pDtStr1 – "1999, Sep 9 08:46:53.744611" (Output.js, line 169)
[Log] pDtStr4 – "1999, Sep 9 08:46:53" (Output.js, line 226)
[Log] milliSecondsSinceEpoch – NaN (Output.js, line 239)
[Log] dateObj – Invalid Date  (Output.js, line 241)
[Log] utcDT1a – "Invalid Date" (Output.js, line 246)
[Log] utcDT1b – "Invalid Date" (Output.js, line 250)
[Log] localDT1a – "Invalid Date" (Output.js, line 271)
[Log] localDT1b – "Invalid Date" (Output.js, line 274)

Would you like me to open a new issue (or issues) for the other things? I'm not familiar with the codebase or your preferences for how specific you would like the issues, but I saw two things:

  • The UTC output on Safari
  • The off by 4 hours (essentially the EDT conversion) between the time shown in the white under the job name and what is shown in the job output.

Thanks again for the help with this!

@erwindon
Copy link
Owner

I should have done a code search when you first mentioned "1999"...
The highstate-task-formatter adds "1999, Sep 9" to the time reported for a task.
This way, conversion is possible between timezones.
But the JS implementation for Mac does not work with that notation.
I've added code to just remove that part when it still remains.

Can you take a quick look whether "1999" is still visible?

@mkbrown
Copy link
Author

mkbrown commented Oct 25, 2022

Looks good now, no 1999. Partying like it's 2022 now...

image

@erwindon erwindon assigned erwindon and unassigned mkbrown Oct 25, 2022
@erwindon
Copy link
Owner

thx for testing!
I'll merge this after a final cleanup

@erwindon erwindon force-pushed the datetime branch 2 times, most recently from 803cabf to 0e23167 Compare October 26, 2022 22:56
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@erwindon erwindon merged commit ead15c8 into master Oct 26, 2022
@erwindon erwindon deleted the datetime branch October 26, 2022 23:00
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.

None yet

2 participants