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

Fix rendering X axis in TraceResultsScatterPlot - pass milliseconds to moment.js #274

Merged
merged 2 commits into from
Nov 18, 2018

Conversation

istrel
Copy link
Contributor

@istrel istrel commented Nov 13, 2018

Resolves #270

Which problem is this PR solving?

This PR fixes rendering X axis values in TraceResultsScatterPlot.

Short description of the changes

Now it passes milliseconds into moment.js function call. Previously there were milliseconds multiplied by 1000 (i.e. raw timestamp from server)

Signed-off-by: Ivan Strelkov <iv-strelkov@yandex.ru>
@codecov
Copy link

codecov bot commented Nov 13, 2018

Codecov Report

Merging #274 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
+ Coverage   77.08%   77.15%   +0.06%     
==========================================
  Files         135      135              
  Lines        2955     2955              
  Branches      613      613              
==========================================
+ Hits         2278     2280       +2     
+ Misses        534      532       -2     
  Partials      143      143
Impacted Files Coverage Δ
...nents/SearchTracePage/SearchResults/ScatterPlot.js 72.72% <100%> (+36.36%) ⬆️
...neViewer/TimelineHeaderRow/TimelineViewingLayer.js 87.03% <0%> (-3.71%) ⬇️

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 feeaccd...78929b0. Read the comment docs.

<XAxis
title="Time"
tickTotal={4}
tickFormat={t => moment(t / ONE_MILLISECOND).format('hh:mm:ss a')}
Copy link
Member

Choose a reason for hiding this comment

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

what does the moment() function do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moment - is the main function of moment.js framework - https://momentjs.com/

It may accept different inputs (strings, Date objects, timestamps). Here it accepts timestamp (number of milliseconds since 1 January 1970 UTC)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

While this may be an OK fix for this specific issue, I think a bigger problem is that we have to do that in the first place. I would expect the UI to have some domain model of the trace and expose the timestamps not in raw microseconds format, but in some structured representation, so that the presentation code wouldn't have to do these conversions all over the place.

@yurishkuro
Copy link
Member

Is it possible to add tests?

@istrel
Copy link
Contributor Author

istrel commented Nov 13, 2018

I guess, It's possible to add tests.

But I have to expose ScatterPlotImpl only for testing purposes to do so.

Signed-off-by: Ivan Strelkov <iv-strelkov@yandex.ru>
@istrel
Copy link
Contributor Author

istrel commented Nov 14, 2018

I've added tests.

Note: In order to test axis rendering, I've exported ScatterPlotImpl.

@istrel
Copy link
Contributor Author

istrel commented Nov 14, 2018

@yurishkuro Could you please tell me what to do next with this PR?

@yurishkuro
Copy link
Member

Thanks!

@tiffon is travelling, should be back later this week, I would like him to review.

@tiffon
Copy link
Member

tiffon commented Nov 18, 2018

Looks great! Thanks for your contribution! 🎉

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.

👍

@tiffon tiffon merged commit e01bc36 into jaegertracing:master Nov 18, 2018
copa2 pushed a commit to copa2/jaeger-ui that referenced this pull request Nov 18, 2018
…o moment.js (jaegertracing#274)

* Fix rendering X axis in TraceResultsScatterPlot

Signed-off-by: Ivan Strelkov <iv-strelkov@yandex.ru>

* Test ScatterPlot axis rendering

Signed-off-by: Ivan Strelkov <iv-strelkov@yandex.ru>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…o moment.js (jaegertracing#274)

* Fix rendering X axis in TraceResultsScatterPlot

Signed-off-by: Ivan Strelkov <iv-strelkov@yandex.ru>

* Test ScatterPlot axis rendering

Signed-off-by: Ivan Strelkov <iv-strelkov@yandex.ru>

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.

ScatterPlot showing incorrect times
3 participants