You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/10/28 15:52:35 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #3056: Properly format timestamps in Accumulo Monitor
cshannon opened a new pull request, #3056:
URL: https://github.com/apache/accumulo/pull/3056
Fixes the `dateFormat()` function to return a formatted timestamp and not the original date.
An example of the new format is: `10/28/2022, 11:49:29 AM EDT`
I kept the existing behavior which will let the browser auto detect the local time zone and display in whatever time zone the user has set on their system which I think is the best approach for a UI system (vs forcing UTC on the user). A future enhancement may be to allow specifying the time zone to display.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295239039
Should we hold off merging this until 2.1.0 is done? It looks like the branch is still set to 2.1.0-SNAPSHOT and not 2.1.1-SNAPSHOT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295854216
@ctubbsii - There are actually quite a few ways we can customize `toLocaleString().` Documentation is located here for [toLocaleString() ](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString) and the options for customization supported are the same as [Intl.DateTimeFormat](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat)
This list is not exhaustive of course but I figure one of these should accomplish the goal of being more concise but still containing all the info we want (date, time, timezone, etc)
Here are several examples of options and outputs, what do you think of them? Any of these seem fine to me. Do we want to include the day of the week? Also do you prefer the timezone as an offset or the actual name? (ie EDT or GMT-4).
<pre>
date.toLocaleString([], { timeStyle: 'long', dateStyle: 'medium' })
Output: <b>Oct 29, 2022, 10:08:11 AM EDT</b>
</pre>
<pre>
date.toLocaleString([], { timeStyle: 'long', dateStyle: 'long' })
Output: <b>October 29, 2022 at 10:04:00 AM EDT</b>
</pre>
<pre>
date.toLocaleString([], { weekday: 'short', month: 'short', day: 'numeric', year: 'numeric',
hour: 'numeric', minute: 'numeric', second: 'numeric', timeZoneName: 'shortOffset' })
Output: <b>Sat, Oct 29, 2022, 10:08:11 AM GMT-4</b>
</pre>
<pre>
date.toLocaleString([], { weekday: 'short', month: 'short', day: 'numeric', year: 'numeric',
hour: 'numeric', minute: 'numeric', second: 'numeric', timeZoneName: 'short' })
Output: <b>Sat, Oct 29, 2022, 10:16:28 AM EDT</b>
</pre>
<pre>
date.toLocaleString([], { weekday: 'short', month: 'numeric', day: 'numeric', year: 'numeric',
hour: 'numeric', minute: 'numeric', second: 'numeric', timeZoneName: 'short' })
Output: <b>Sat, 10/29/2022, 10:16:28 AM EDT</b>
</pre>
<pre>
date.toLocaleString([], { month: 'numeric', day: 'numeric', year: 'numeric',
hour: 'numeric', minute: 'numeric', second: 'numeric', timeZoneName: 'short' })
Output: <b>10/29/2022, 10:17:41 AM EDT</b>
</pre>
<pre>
date.toLocaleString([], { weekday: 'short', month: '2-digit', day: '2-digit', year: '2-digit',
hour: 'numeric', minute: 'numeric', second: 'numeric', timeZoneName: 'shortOffset' })
Output: <b>Sat, 10/29/22, 10:17:41 AM GMT-4</b>
</pre>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295351810
@ctubbsii and @EdColeman , My latest commit updates things so it's YYYY-MM-DD and Z since I used the toISOString() format and that's in Z, new sample screen shot:
Let me know what you think, I can always write something custom to tweak things more because as I said out of the box there's not a lot of formatting options in JavaScript (Java has a lot more customization options with its formatter naively)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] AlbertWhitlock commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
AlbertWhitlock commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295476696
I reviewed these changes. They look good. Displaying as:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1298313380
I pushed up the latest change with the new format. Screenshot of the latest:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295598517
@ctubbsii - So there is actually one bug here either way. The current code is not actually using the ` toLocaleString()`. It calls that method but it returns the original date so what you are seeing as actually the format of the `toString()` method. I do agree that I liked having the locality better (your own time zone).
If we fixed the bug and used the `toLocaleString()` method for real it wouldshow up as:
`10/28/2022, 6:45:19 PM`
If you like the current format then I can just fix the bug and remove the unnecessary call to toLocaleString() and then maybe drop the full time zone name as I still think that we should at least drop "Eastern Daylight Time" as it's so verbose.
**New Proposed format:**
`Fri Oct 28 2022 18:47:02 GMT-0400`
How does that sound?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295628730
I do prefer your proposed format. However, I'm also thinking it's probably best to just stick with the default locale string, even though I don't personally like it. It seems to be the option that gives the most deference to the user's environment/configuration. I hadn't realized that was an actual bug. I thought this was just about style preferences. My main concern is that it doesn't show the time zone (neither do the logs, apparently, since the version of ISO8601 log4j is printing doesn't seem to include it, so maybe it's fine).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1297262925
> > > date.toLocaleString([], { timeStyle: 'long', dateStyle: 'medium' })
> > > Output: Oct 29, 2022, 10:08:11 AM EDT
> >
> >
> > Out of the ones listed, I'd put my vote on something like this. Clear on month and day to avoid any possible confusion but also short and concise. EDT over GMT +/- though not sure how useful timezone is in general for these logs.
>
> I agree with this.
I'll update the PR with this later today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1297028318
> > date.toLocaleString([], { timeStyle: 'long', dateStyle: 'medium' })
> > Output: Oct 29, 2022, 10:08:11 AM EDT
>
> Out of the ones listed, I'd put my vote on something like this. Clear on month and day to avoid any possible confusion but also short and concise. EDT over GMT +/- though not sure how useful timezone is in general for these logs.
I agree with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
Manno15 commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295870397
> date.toLocaleString([], { timeStyle: 'long', dateStyle: 'medium' })
Output: Oct 29, 2022, 10:08:11 AM EDT
Out of the ones listed, I'd put my vote on something like this. Clear on month and day to avoid any possible confusion but also short and concise. EDT over GMT +/- though not sure how useful timezone is in general for these logs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295256770
I will again toss out that there is benifits of matching the timestamp format of the logs `2022-10-28 17:13:50,493` - yes, that can be changes, but by default if we are consistent then it eases things.
An example would be grab a timestamp from the monitor and then grep a log - much easier to cut-n-paste than needing to convert and type.
But anything that makes the timestamp shorter is moving in the right direction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon merged pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon merged PR #3056:
URL: https://github.com/apache/accumulo/pull/3056
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295638933
Alright, tomorrow I can fix it to use toLocaleString() like it was supposed to be all along. I will also do some research/testing and see if we can customize the output at all. I think the function takes some options that we can set so we may be able to customize the output to show the time zone, but not sure until I try.
If we can customize it I can give some example outputs and pick the one everyone likes the best otherwise I'll just use what's given if we can't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295286004
> I would strongly prefer not having MM/dd/YYYY format. It is very confusing to me, having a military background, and living overseas for a bit. YYYYMMdd is better, because it's sortable, and europeans use dd/MM/YYYY, which is often indistinguishable from MM/dd/YYYY, enough to cause confusion.
>
> The human-readable long format is better in that regard... no ambiguity or confusion. Something like dd-MMM-YYYY might also be okay (e.g. 04-JUL-1776), but please avoid anything that results in MM/dd/YYYY.
We may have to go ahead and use the `toISOString() `method and then parse and customize that a bit. Using toLocaleString() doesn't let you change the year to be first, which makes sense, as the it's supposed to format for your locality. As you mentioned it would show up different for Europeans.
I could just show something like `2011-10-05 14:48:00.000Z` which is what @EdColeman had mentioned. If we wanted to keep local timezone and AM/PM then we'd need to do more work or find another library as I don't think native Javascript has a lot of options out of the box for formatting timestamps so just using ISO in this case probably makes the most sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3056: Properly format timestamps in Accumulo Monitor
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #3056:
URL: https://github.com/apache/accumulo/pull/3056#issuecomment-1295251919
> @ctubbsii - Should we hold off merging this until 2.1.0 is done? It looks like the branch is still set to 2.1.0-SNAPSHOT and not 2.1.1-SNAPSHOT
Things can be merged into the 2.1 branch, but they generally should be limited to bug fixes (semver rules). If we need another release candidate, they will make it into 2.1.0. If not, they'll make it into 2.1.1. The current SNAPSHOT version doesn't matter. We'll update those after the release is finished, as needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org