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

Container dashboard charts: send datapoints as full timestamps #3696

Merged

Conversation

mzazrivec
Copy link
Contributor

We need to send chart datapoints as complete timestamps (including timezone info) so that the patternfly component would correctly convert them to Date() objects and avoid ugly side-effects when viewing the charts in browsers located in different timezones.

How to reproduce:

  1. Have Container provider with some utilization data collected, let's say your data is being collected every midnight in EST timezone.
  2. Check the container provider dashboard in CEST timezone
  3. Check the container provider dashboard in EST timezone: $ TZ='America/New_York' firefox

You'll notince the datapoints in the EST timezone are shifted by one day back.

Previously, we were sending the datapoints to the angular component as 2018-03-23 strings. The patternfly component would then convert them to Date() objects. Now we'll be sending the full timestamp as 2018-03-23T00:00:00.0000-04:00.

https://bugzilla.redhat.com/show_bug.cgi?id=1542720

@himdel review please?

@@ -233,7 +233,7 @@ def hourly_image_metrics
def daily_image_metrics
daily_image_metrics = Hash.new(0)
daily_metrics.each do |m|
day = m.timestamp.strftime("%Y-%m-%d")
day = m.timestamp
Copy link
Contributor

@himdel himdel Apr 5, 2018

Choose a reason for hiding this comment

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

This looks problematic.

It gives me "Sat, 15 Jul 2017 22:29:00 UTC +00:00" .. which is not guaranteed to be parseable in javascript IIRC

Should this be m.timestamp.strftime("%Y-%m-%dT%H:%M:%S%z")?

EDIT: never mind..

> m.timestamp
=> Sat, 15 Jul 2017 22:29:00 UTC +00:00
> m.timestamp.to_s
=> "2017-07-15 22:29:00 UTC"

@himdel
Copy link
Contributor

himdel commented Apr 5, 2018

@mzazrivec Looks like we will have to fix this differently.

The problem is that previously, all data points from one day were summed, so you'd get

2016-01-01: 42

but now, you'll get data more like:

2016-01-01T10:20:11: 2,
2016-01-01T12:24:11: 20,
2016-01-01T14:44:11: 10,
2016-01-01T17:04:11: 10,

which is not summed by day.

Also...

before:

master

after:

pr3696

@himdel
Copy link
Contributor

himdel commented Apr 5, 2018

@mzazrivec Maybe we can do the same thing we do for hours?

hour = m.timestamp.beginning_of_hour.utc

so, something like date = m.timestamp.beginning_of_day.utc?

@himdel
Copy link
Contributor

himdel commented Apr 5, 2018

Using this diff instead:

diff --git a/app/services/container_dashboard_service.rb b/app/services/container_dashboard_service.rb
index d4bf6ac6c..2a95bf09b 100644
--- a/app/services/container_dashboard_service.rb
+++ b/app/services/container_dashboard_service.rb
@@ -233,7 +233,7 @@ class ContainerDashboardService < DashboardService
   def daily_image_metrics
     daily_image_metrics = Hash.new(0)
     daily_metrics.each do |m|
-      day = m.timestamp.strftime("%Y-%m-%d")
+      day = m.timestamp.beginning_of_day.utc
       daily_image_metrics[day] +=
         m.stat_container_image_registration_rate if m.stat_container_image_registration_rate.present?
     end
diff --git a/app/services/container_service_mixin.rb b/app/services/container_service_mixin.rb
index 8ee70b172..d9012e2e6 100644
--- a/app/services/container_service_mixin.rb
+++ b/app/services/container_service_mixin.rb
@@ -10,7 +10,7 @@ module ContainerServiceMixin
     daily_pod_delete_trend = Hash.new(0)
 
     daily_metrics.each do |m|
-      date = m.timestamp.strftime("%Y-%m-%d")
+      date = m.timestamp.beginning_of_day.utc
       fill_pod_metrics(m, date, daily_pod_create_trend, daily_pod_delete_trend)
     end
 
@@ -122,7 +122,7 @@ module ContainerServiceMixin
   def daily_network_metrics
     daily_network_metrics = Hash.new(0)
     daily_metrics.each do |m|
-      day = m.timestamp.strftime("%Y-%m-%d")
+      day = m.timestamp.beginning_of_day.utc
       daily_network_metrics[day] += m.net_usage_rate_average if m.net_usage_rate_average.present?
     end
 
@@ -179,7 +179,7 @@ module ContainerServiceMixin
     total_mem = Hash.new(0)
 
     daily_metrics.each do |metric|
-      date = metric.timestamp.strftime("%Y-%m-%d")
+      date = metric.timestamp.beginning_of_day.utc
       fill_utilization(metric, date, used_cpu, used_mem, total_cpu, total_mem)
     end
 

beginning

(but not yet sure this still fixes the BZ...) (seems it does, at least the screenshot is the same for TZ='America/New_York' firefox and TZ=UTC firefox)

@mzazrivec mzazrivec force-pushed the fix_datepoints_in_container_dashboard_charts branch 2 times, most recently from c17420a to 4cd0fae Compare April 6, 2018 09:24
@mzazrivec
Copy link
Contributor Author

@himdel fixed as suggested.

@@ -233,7 +233,7 @@ def hourly_image_metrics
def daily_image_metrics
daily_image_metrics = Hash.new(0)
daily_metrics.each do |m|
day = m.timestamp.strftime("%Y-%m-%d")
day = m.timestamp.timestamp.beginning_of_day.utc
Copy link
Contributor

@himdel himdel Apr 6, 2018

Choose a reason for hiding this comment

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

double timestamp ;)

(otherwise, LGTM)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

We need to send chart datapoints as complete timestamps (including
timezone info) so that the patternfly component would correctly
convert them to Date() objects and avoid ugly side-effects
when viewing the charts in browsers located in different timezones.

https://bugzilla.redhat.com/show_bug.cgi?id=1542720
@mzazrivec mzazrivec force-pushed the fix_datepoints_in_container_dashboard_charts branch from 4cd0fae to 35f2268 Compare April 6, 2018 17:32
@miq-bot
Copy link
Member

miq-bot commented Apr 6, 2018

Checked commits mzazrivec/manageiq-ui-classic@044b120~...35f2268 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@himdel himdel merged commit 43e6773 into ManageIQ:master Apr 6, 2018
@himdel himdel added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 6, 2018
@mzazrivec mzazrivec deleted the fix_datepoints_in_container_dashboard_charts branch April 7, 2018 09:22
simaishi pushed a commit that referenced this pull request Apr 9, 2018
…ashboard_charts

Container dashboard charts: send datapoints as full timestamps
(cherry picked from commit 43e6773)

https://bugzilla.redhat.com/show_bug.cgi?id=1565156
@simaishi
Copy link
Contributor

simaishi commented Apr 9, 2018

Gaprindashvili backport details:

$ git log -1
commit f7016d9ba5a4cb7645318fb41dc317d35e3645ba
Author: Martin Hradil <himdel@seznam.cz>
Date:   Fri Apr 6 20:08:16 2018 +0200

    Merge pull request #3696 from mzazrivec/fix_datepoints_in_container_dashboard_charts
    
    Container dashboard charts: send datapoints as full timestamps
    (cherry picked from commit 43e677321cc7330a53e7cad749b388fc28590c21)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1565156

simaishi pushed a commit that referenced this pull request Apr 10, 2018
…ashboard_charts

Container dashboard charts: send datapoints as full timestamps
(cherry picked from commit 43e6773)

https://bugzilla.redhat.com/show_bug.cgi?id=1565157
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 4f0261dd6852c58989f6121e6535c1856f1e8b39
Author: Martin Hradil <himdel@seznam.cz>
Date:   Fri Apr 6 20:08:16 2018 +0200

    Merge pull request #3696 from mzazrivec/fix_datepoints_in_container_dashboard_charts
    
    Container dashboard charts: send datapoints as full timestamps
    (cherry picked from commit 43e677321cc7330a53e7cad749b388fc28590c21)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1565157

@simaishi simaishi removed the fine/yes label Apr 10, 2018
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

4 participants