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

Bugs in the average metrics graph #2874

Merged
merged 7 commits into from
Jan 21, 2018

Conversation

uxen-ab
Copy link
Contributor

@uxen-ab uxen-ab commented Jan 17, 2018

Problems

  1. The suffix wasn't displayed in the tooltip when mouseovering a metric point.
  2. Metric points added in the future could be displayed on the graph.
  3. When using the "Last 12 hours" view if it was less than 12pm the last hours of the previous day were displayed, but ChartJS put them at the end of the graph.

The second point could seems useless, but since there are some problems with timezone when posting a metric point and the date when it's stored, it's possible to have a shift between the real date of posting, and the date stored/displayed.

How they were fixed

  1. The JavaScript syntax was fixed in order to execute the "tootip" callback.
  2. A AND created_ad < NOW() was added in the different syntaxes for the different repositories in order to filter the future.
  3. Select the datetime instead of only the time of every metric point and then cut this string in the tooltip and legend, in order to CharJS be able to order them.

When selecting the time of metric's points only the time was selected,
not the date. The issue with this is mainly in the view "Last 12 hours".
Example:
It's 11:10 am and I choose the "Last 12 hours" view, the metrics points
will be from 23:00 yesterday to 11:00 am today.
When giving all these datas and labels to ChartJS, it sorts the points
by label from lower to highter. It means 23:00 (from yesterday) will be
after the datas of today, it doesn't have sense.

To fix it, I've added in the repositories the date in addition to the
time. So it's no longer 11:00 that is selected but 2018-01-15 11:00.
I've updated the Metric vue in order to cut the label when displaying so
it doesn't change the displaying.
Because there are metric views that are based on date but not time,
there is a condition in the Metric vue to cut the string only if the
time is present.

Related to cachethq#2848
When we mouseovered on a point on a metric, the value was shown but the
suffix wasn't.
It was due to a curly bracket issue, the "tooltip" option wasn't given
as an "option" sub-object, but as a third argument of the Chart
constructor. A curly bracket was closed to early.
This is fixed and the suffix is now displayed.

Related to cachethq#2848
The metrics had problem with the points, if I post/save a metric point
that is created in the future, it was selected.

Example:
It is 10:00 am, I post or save a metric point and it's 'created at'
10:30 am. Using the "Last 12 hours" the metric point would be selected.

This update is applied on all the SQL queries, on minutes, hours, days.

Related to cachethq#2848
Update from upstream repo CachetHQ/Cachet
@jbrooksuk
Copy link
Member

This looks great, thanks!

@jbrooksuk jbrooksuk merged commit 2783824 into cachethq:2.4 Jan 21, 2018
@uxen-ab uxen-ab deleted the bugs-in-the-average-metrics-graph branch January 22, 2018 09:48
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

2 participants