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

Adds a configuration option to customize the standard datetime format #403

Closed
wants to merge 6 commits into from

Conversation

leogomes
Copy link
Contributor

@leogomes leogomes commented Jul 2, 2019

Signed-off-by: Leonardo FREITAS GOMES leonardo.f.gomes@gmail.com

Which problem is this PR solving?

  • Current datetime format used is not precise. It shows the trace start time with a minute precision.

Short description of the changes

  • Adds a new configuration option that allows users to specify the datetime format they want to use, so that users can display time using format options like fractional second and timezone.

… used.

Signed-off-by: Leonardo FREITAS GOMES <leonardo.gomes@amadeus.com>
@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #403 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
+ Coverage   91.58%   91.63%   +0.05%     
==========================================
  Files         176      176              
  Lines        3994     3995       +1     
  Branches      921      922       +1     
==========================================
+ Hits         3658     3661       +3     
+ Misses        298      296       -2     
  Partials       38       38
Impacted Files Coverage Δ
...ackages/jaeger-ui/src/constants/default-config.tsx 100% <ø> (ø) ⬆️
packages/jaeger-ui/src/utils/date.tsx 90.47% <100%> (ø) ⬆️
packages/jaeger-ui/src/constants/index.tsx 100% <100%> (ø) ⬆️
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx 91.52% <0%> (+3.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b59893...1ff8ee5. Read the comment docs.

@yurishkuro
Copy link
Member

Can you provide a screenshot? Which timestamps is this affecting?

Also, there should be some documentation about the config file format, it needs to be updated to describe the new property.

@leogomes
Copy link
Contributor Author

leogomes commented Jul 3, 2019

Here's a possible new date format set:
image

and here's the original one:
image

This only affects this timestamp now. With my change, the STANDARD_DATETIME_FORMAT is customized, so it could affect other dates in the future. So far, it seems to be only used by the "Trace Start" header field.

@leogomes
Copy link
Contributor Author

leogomes commented Jul 3, 2019

For documentation update, it would be a separate PR here: https://github.com/jaegertracing/documentation/blob/master/content/docs/next-release/frontend-ui.md

right, @yurishkuro ?

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a config value, maybe it makes sense to change the trace page header to show a more precise datetime, instead? What is your preferred level of precision?

I think this change looks good but the use of the word "standard" is out of place.

@@ -66,6 +66,7 @@ export default deepFreeze(
gaID: null,
trackErrors: true,
},
standardDatetimeFormat: FALLBACK_STANDARD_DATETIME_FORMAT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is used in only one place, I don't think it's the "standard" date time format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called it like that, because it overrides the format used by the STANDARD_DATETIME_FORMAT constant. Today, it's only used in one place, but if other bits of the UI start using https://github.com/jaegertracing/jaeger-ui/blob/master/packages/jaeger-ui/src/utils/date.tsx#L64 they will also be affected. I'm fine with calling it "traceStartDateFormat" too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffon What do you suggest? Shall I remove the config option and add the fractional seconds to the format as shown below? Or should I keep the config and just rename the variable to remove the standard_ prefix?

@leogomes
Copy link
Contributor Author

leogomes commented Jul 6, 2019

Instead of adding a config value, maybe it makes sense to change the trace page header to show a more precise datetime, instead? What is your preferred level of precision?

Up to fractional seconds would be a nice format: 'MMMM Do YYYY, HH:mm:ss.SSS'
image

@tiffon
Copy link
Member

tiffon commented Jul 10, 2019

@yurishkuro The fractional seconds work for you? I think it looks good and is preferable over another config option.

@yurishkuro
Copy link
Member

It's fine. Using smaller font for decimals is another option (but harder to implement).

@tiffon
Copy link
Member

tiffon commented Jul 11, 2019

@leogomes Thanks for your help with this.

Reducing the contrast might be preferable over making the text smaller; it's already pretty small.

Screen Shot 2019-07-10 at 11 41 16 PM

That was done using the u-tx-muted util CSS class:

  {
    key: 'timestamp',
    label: 'Trace Start',
    renderer: (trace: Trace) => {
      const dateStr = formatDatetime(trace.startTime);
      const match = dateStr.match(/^(.+)(\.\d+)$/);
      return match ? <>{match[1]}<span className="u-tx-muted">{match[2]}</span></> : dateStr;
    }
  },

@leogomes
Copy link
Contributor Author

leogomes commented Aug 1, 2019

Hello @tiffon, @yurishkuro,

What's the status on this one? Have @tiffon changes been merged somewhere and we can close this one?

Thanks,
Leo

@yurishkuro
Copy link
Member

@tiffon any more feedback?

tiffon added a commit to tiffon/jaeger-ui that referenced this pull request Aug 4, 2019
Signed-off-by: Joe Farro <joef@uber.com>
tiffon added a commit to tiffon/jaeger-ui that referenced this pull request Aug 4, 2019
Signed-off-by: Joe Farro <joef@uber.com>
@tiffon
Copy link
Member

tiffon commented Aug 4, 2019

@leogomes Thanks. I put together a PR for it: #430

LMK if you have any feedback.

I think this PR is good to close.

@tiffon tiffon closed this Aug 4, 2019
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants