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

SaltGUI shows wrong start time in job list #430

Merged
merged 8 commits into from
Apr 12, 2022
Merged

Conversation

erwindon
Copy link
Owner

Describe the bug
The "Start Time" column of Jobs in SaltGUI is off by one hour.
This is probably related to the unsed time zone.

The time is shown correctly in the Salt output. Thus, I think the problem is with SaltGUI.

To Reproduce

  • We have a time zone set on our server: sudo timedatectl set-timezone Europe/Paris
  • When running a job, then salt shows the correct start time, for example:
done, 1 response
my-minion:▽●

✔echo "Hello Bob" id=say_hello (from hello.sls)
    Function is cmd.run
    Command "echo "Hello Bob"" run
    pid: 14384
    retcode: 0
    stderr: ""
    stdout: "Hello Bob"
    Started at 10:36:48.076396
    Duration 1.172 s
1 succeeded, 4 changes
  • When viewing this Job in SaltGUI, the "Start Time" column says: 2022, Mar 14 09:36:44.728128, which is off by approx. one hour

Expected behaviour
The "Start Time" column in SaltGUI should show the same value as the output from salt.

Is there maybe a time zone setting in SaltGUI to fix this?

@achimmihca achimmihca added the bug label Mar 14, 2022
@achimmihca achimmihca changed the title SaltGUI shows wrong time in Job-List SaltGUI shows wrong start time in job list Mar 14, 2022
@erwindon
Copy link
Owner

Your statement is most probably correct.
I´ll look into it further in about a week.

@erwindon
Copy link
Owner

The following datetime instances are already recognized by SaltGUI.

  • [job].start_time
  • [task].start_time
  • [event]._stamp
  • [beacon]._stamp

Note that [event]._stamp already has some timezone logic applied.

Note that any datetimes inside any payload data will not be adjusted for timezone.

Idea: can we show the date with a tooltip that shows the datetime in alternative timezones?

@achimmihca
Copy link
Contributor Author

Idea: can we show the date with a tooltip that shows the datetime in alternative timezones?

I dont understand, why not show the datetime for the proper timezone in the table?

Since salt is showing the correct datetime for my timezone in the output, I would expect that SaltGUI is receiving this value and thus can show the same string.
Or is salt sending the datetime in UTC? Is there maybe a way to get the timezone from salt?

@erwindon
Copy link
Owner

I dont understand, why not show the datetime for the proper timezone in the table?

that is the intention.
the suggestion was about adding a tooltip to that, showing the (original) UTC value.
this compensates for the timestamp values that are inside the return-value data, which are not recognized (and thus not TZ corrected)

@achimmihca
Copy link
Contributor Author

Ahh I see, yes beeing able to see the original UTC value is a good idea.

@achimmihca
Copy link
Contributor Author

Can I help you with this?

@erwindon
Copy link
Owner

Since all other work on SaltGUI has been handled now, this is what I'm working on now.
But I did not push any work yet.
One of the main problems (or work) is that usually only the formatted date is available, so it must be parsed first.
This might be better in the original python code, but json just does not have a datettime type.
I expect this to be ready in a few days.

@erwindon
Copy link
Owner

The first implementation will probably not involve a tooltip to show an alternative value. Instead, the existing function Output.dateTimeStr will provide the combined representation as text.
Might look like: 2022, Mar 30 20:43:53 (22:43:53 CEST) when the original representation is used, or like: 30-3-2022 20:43:53 (22:43:53 CEST) (in NL) or like 30.3.2022 20:43:53 (22:43:53 MESZ) (in D) when the local representations are used.
Or just like 20:43:53 (22:43:53 CEST) when only the time part is shown.
Note that the timezone-codes seems to be strangely implemented in browsers, it may show CEST or GMT+2 depending on brand/version of the browser and/or the region that is involved.

As noted before, the existing logic for the formatting of event timestamps with be removed.

@achimmihca
Copy link
Contributor Author

Instead, the existing function Output.dateTimeStr will provide the combined representation as text.

Works for me

@erwindon
Copy link
Owner

erwindon commented Mar 31, 2022

(the issue is converted to a PR from my repo)

  • datetime (or time) fields are now re-formatted in the local representation, but still utc
  • the representation includes the preferred decimal separator (see https://en.wikipedia.org/wiki/Decimal_separator)
  • fields show additional text to show the local time with the abbreviated timezone, e.g. 31-3-2022 13:37:27,454226 (15:37:27 CEST). The date may also be different, but that is not shown. The milliseconds are also left out.
  • add a setting in the master file on how to format the date/time fields
  • add a section in the Options page to show that setting. access the (otherwise hidden) Options using ctrl-click on the SaltGUI logo. the page allows to change some settings for the duration of the session.
  • the variations are:
    • utc (no TZ visible) this is the old default, remains the default
    • localtime (TZ visible)
    • utc with localtime (TZ visible for localtime)
    • localtime with utc (TZ visible for localtime)
  • added a tooltip with representation for both utc and localtime, also shows the full amount of fractional digits for seconds. this only works for independent datetime fields, when the datetime value is part of other text, the tooltip does not appear.
  • updated beacons-per-minion screen to show the timestamp in a separate columns and without date information. previously, the datetime was shown near the beacon-value. since the datetime field is now separate, the tooltip is visible for it.

@achimmihca can you take a look at this intermediate result?

@erwindon erwindon marked this pull request as draft March 31, 2022 14:24
@erwindon erwindon force-pushed the timepresentation branch 2 times, most recently from dc8434f to fa6980a Compare March 31, 2022 22:04
@erwindon erwindon assigned achimmihca and unassigned erwindon Apr 1, 2022
Repository owner deleted a comment from sonarcloud bot Apr 2, 2022
@erwindon
Copy link
Owner

erwindon commented Apr 2, 2022

still working on lower priority stuff in the same area:

  • use the new datetime logic in the "Options" screen
    • options-panel: for session_start and session_expire
    • stats-panel: for "Current Time", "Start Time" and "Uptime" on top-level
    • stats-panel: for "Current Time", "Start Time" in requests
    • stats-panel: for "Current Time", "Start Time" in slow-queries
  • use the new datetime logic also in highstate overviews (start-time)

I cannot do much work next week, so progress on the extra items will be slow...

@achimmihca
Copy link
Contributor Author

achimmihca commented Apr 4, 2022

I just tested this. I get a tooltip that shows the UTC and local time.
But I get the same result no matter how I set saltgui_datetime_representation. The column always shows utc and the tooltip the utc and local values.

What I expected is: The column shows the local time and the tooltip optionally the utc time as well when I set it to local+utctime.

But still: local time in tooltip is correct.
I guess the rest will be implemented when you find time?

Thanks!

2022-04-04 09_52_10-SaltGUI

@erwindon
Copy link
Owner

erwindon commented Apr 5, 2022

@achimmihca
the content of the tooltip is always the same (utc+local, with full time resolution), so that is correct here.
all datetime representations are with the local rules (e.g. dot between day and month and comma as decimal separator, both as usual in your country), so that is correct too.
You should be able to choose the application datetime-format using the master configuration file or overrule using the Options page. I will look into that part in a few days...
Thx so far!

@erwindon erwindon assigned erwindon and unassigned achimmihca Apr 5, 2022
@erwindon
Copy link
Owner

@achimmihca
I fixed the setting of the date+time representation.
You should be able to select between the 4 variations again.
I simplified the values of the setting, replacing the previous + with a -, so that the values can be used also as part of an html-element-id.

Can you please try again?

@erwindon erwindon assigned achimmihca and unassigned erwindon Apr 11, 2022
@erwindon erwindon marked this pull request as ready for review April 12, 2022 07:49
@erwindon
Copy link
Owner

@achimmihca
just for completeness, can you try again with unmodified code?

@achimmihca
Copy link
Contributor Author

@erwindon , I don't see any change?
JSON.stringify is still there.

Did you push anything?

@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 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 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@erwindon
Copy link
Owner

Did you push anything?

ehh, no, somehow missed that, done that now...

@achimmihca
Copy link
Contributor Author

Now its working 😉 .
Many thanks!

@erwindon erwindon merged commit 1aed4c2 into master Apr 12, 2022
@erwindon erwindon deleted the timepresentation branch April 16, 2022 14:34
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