Fixes #23665 - Use Patternfly charts in Runtime #5600

boaz0 · 2018-05-22T08:07:15Z

The flot package is deprecated although it's being used
to plot charts in Foreman.

One of those charts that uses flot is the Runtime chart.
In this commit:

Added a new component called Timeseries chart that uses Patternfly charts
Rendered the component on "hosts/show.html.erb"
Added unit tests to test the component.
Removed unnecessary files such as "hosts/_runtime.html.erb"
Changed the "hosts/:name/runtime" routes to return JSON

Before

After

theforeman-bot · 2018-05-22T08:07:24Z

Issues: #23665

boaz0 · 2018-05-22T08:07:49Z

/cc @ohadlevy @sharvit @amirfefer @lizagilman PTAL
Thanks

ohadlevy

initial code review, did not test really as its not mountable.

Also would be happy to see storybook examples on how to use the time series charts.

ohadlevy · 2018-05-22T09:04:51Z

app/controllers/hosts_controller.rb

@@ -300,7 +300,7 @@ def vm
  end

  def runtime
-    render :partial => 'runtime'
+    render :json => helpers.runtime_chart(params["range"].empty? ? 7 : params["range"].to_i)
  rescue ActionView::Template::Error => exception


I guess the exception is no longer valid?

ohadlevy · 2018-05-22T09:05:50Z

app/helpers/hosts_helper.rb

@@ -229,7 +229,10 @@ def runtime_chart(timerange = 1.day.ago)
      config  << [r.reported_at.to_i*1000, r.config_retrieval]
      runtime << [r.reported_at.to_i*1000, r.runtime]
    end
-    [{:label=>_("Config Retrieval"), :data=> config, :color=>'#AA4643'}, {:label=>_("Runtime"), :data=> runtime, :color=>'#4572A7'}]
+    {:results => [
+      {:label=>_("Config Retrieval"), :data=> config, :color=>'#AA4643'},


i wonder if we need to keep the color overrides, which colors do you get without defining it?

#0088ce ,#39a5dc, #3f9c35 and #ec7a08 which are the defaults in c3js

ohadlevy · 2018-05-22T09:06:38Z

app/views/hosts/show.html.erb

-      <div class="chart" data-ajax-url='<%= runtime_host_path(@host, :range => @range) %>' data-on-complete='updateChart'>
-        <%= spinner(_('Loading runtime information ...')) %>
-      </div>
+      <div id="runtime_react"></div>


not sure if react is needed as part of the div, just host_runtime or something similar would suffice

ohadlevy · 2018-05-22T09:07:43Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/Normalizer.js

@@ -0,0 +1,24 @@
+


nitpick: extra empty line?

ohadlevy · 2018-05-22T09:08:12Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/Normalizer.js

+  return c3Resp;
+};
+
+export default Normalizer;


is it safe to assume this will work the same with trends too?

Yes, looking at the returned value from chart_data, it can normalize the trends too.

ohadlevy · 2018-05-22T09:08:41Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/TimeseriesChart.js

+import { noop } from '../../common/helpers';
+import Normalizer from './Normalizer';
+
+


nitpick: extra line

ohadlevy · 2018-05-22T09:09:22Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/TimeseriesChart.js

+    } = this.props;
+
+    if (hasError) {
+      return <Alert>__("Failed loading chart data")</Alert>;


can we reuse chartbox ? it has a loader and a failure state afair?

ChartBox does more than what TimeseriesChart does.
I find Loader which is used by ChartBox reusable in here too, though.

Thanks for the help.

ohadlevy · 2018-05-22T09:19:09Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/TimeseriesChart.js

+      axis={{ x: { type: 'timeseries', tick: { fit: false } } }}
+      tooltip={{ grouped: false, format: { title: d => new Date(d).toUTCString() } }}
+      zoom={{ enabled: true }}
+      />


I'm missing an empty state representation, currently it looks like this

on second look, it looks like you forgot to add the changes to the mounter?

ohadlevy · 2018-05-22T09:24:42Z

app/views/hosts/show.html.erb

    </div>
+


please configure your editor to remove trailing whitespaces, while not visible at github, this include 4 spaces charts (easily viewable with git diff)

amirfefer

Thanks @ripcurld !
I'm getting a PG error when a details host page renders.
more details in the inline comments.

amirfefer · 2018-05-22T09:13:08Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/TimeseriesChart.js

+    } = this.props;
+
+    if (hasError) {
+      return <Alert>__("Failed loading chart data")</Alert>;


Should be wrapped by {}

amirfefer · 2018-05-22T09:29:54Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/Normalizer.js

+  if (colorItems.length === 0) {
+    return c3Resp;
+  }
+  const colorReducer = (acc, cur) => Object.assign(acc, { [cur.label]: cur.color });


be aware that acc is going to be mutated, if you'd like just a copy, use Object.assign({}, copied data), or use object spread operator, which I tend to prefer because of its readability.

amirfefer · 2018-05-22T09:33:44Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/TimeseriesChart.js

+
+
+class TimeseriesChart extends React.Component {
+  componentWillMount() {


API calls should be in componentDidMount

amirfefer · 2018-05-22T09:41:31Z

webpack/assets/javascripts/react_app/components/TimeseriesChart/TimeseriesChart.js

+    return (
+      <Spinner loading={isLoadingData} size='md'>
+      <PfLineChart
+      data={{ ...data, type: chartType }}


Why not using a service module like donut chart does with ChartService ?

amirfefer · 2018-05-22T09:46:37Z

...ts/react_app/components/TimeseriesChart/__tests__/__snapshots__/TimeseriesChart.test.js.snap

+    className="line-chart-pf"
+    color={
+      Object {
+        "pattern": Array [


is that the default pf colors scheme? or some random colors?

Yes it is or more likely pf is using c3js default color scheme

amirfefer · 2018-05-22T09:49:55Z

app/views/hosts/show.html.erb

-        <%= spinner(_('Loading runtime information ...')) %>
-      </div>
+      <div id="runtime_react"></div>
+      <%= mount_react_component('TimeseriesChart', '#runtime_react', {


doesn't work with PG:
i'm getting this :

( the error string is not extracted )

with this error:

ActiveRecord::StatementInvalid (PG::UndefinedFunction: ERROR: operator does not exist: timestamp without time zone > integer | LINE 1: ...t') AND "reports"."host_id" = $1 AND (reported_at > 7) ORDER... | ^ | HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts. | : SELECT "reports".* FROM "reports" WHERE "reports"."type" IN ('ConfigReport') AND "reports"."host_id" = $1 AND (reported_at > 7) ORDER BY "reports"."reported_at" ASC):

boaz0 · 2018-05-30T06:33:10Z

[test foreman] 🤞

ohadlevy · 2018-05-30T07:34:51Z

@ripcurld can you please add it to the storybook and publish it somehwhere? thanks!

boaz0 · 2018-05-30T08:36:23Z

Sure thing. 👍

houndci-bot · 2018-07-09T07:48:32Z

app/helpers/hosts_helper.rb

-      link_to_function(icon_text('random'), 'randomizeName()', :class => 'btn btn-default',
-        :title => _('Generate new random name. Visit Settings to disable this feature.'))
-    end
+    link_to_function(icon_text('random'), 'randomizeName()', :class => 'btn btn-default',


Style/MultilineIfModifier: Favor a normal if-statement over a modifier clause in a multiline statement.

houndci-bot · 2018-07-09T07:48:32Z

app/helpers/hosts_helper.rb

    else
-      status = '<i class="glyphicon glyphicon glyphicon-arrow-down interface-down" title="' + _('Interface is down') + '"></i>'
+      status = '<i class="glyphicon glyphicon glyphicon-arrow-down interface-down" title="'+ _('Interface is down') +'"></i>'


Layout/SpaceAroundOperators: Surrounding space missing for operator +.

houndci-bot · 2018-07-09T07:48:32Z

app/helpers/hosts_helper.rb

@@ -446,9 +439,9 @@ def link_status(nic)
    return '' if nic.new_record?

    if nic.link
-      status = '<i class="glyphicon glyphicon glyphicon-arrow-up interface-up" title="' + _('Interface is up') + '"></i>'
+      status = '<i class="glyphicon glyphicon glyphicon-arrow-up interface-up" title="'+ _('Interface is up') +'"></i>'


Layout/SpaceAroundOperators: Surrounding space missing for operator +.

houndci-bot · 2018-07-09T07:48:32Z

app/helpers/hosts_helper.rb

@@ -354,7 +347,7 @@ def host_title_actions(host)
          link_to_if_authorized(_("Run puppet"), hash_for_puppetrun_host_path(:id => host).merge(:auth_object => host, :permission => 'puppetrun_hosts'),
                                :disabled => !Setting[:puppetrun],
                                :class => 'btn btn-default',
-                                :title => _("Trigger a puppetrun on a node; requires that puppet run is enabled"))
+                                :title    => _("Trigger a puppetrun on a node; requires that puppet run is enabled"))


Layout/SpaceAroundOperators: Operator => should be surrounded by a single space.

houndci-bot · 2018-07-09T07:48:32Z

app/helpers/hosts_helper.rb

-        { :days => select(nil, 'range', 1..number_of_days,
-                    {:selected => @range}, {:style => "float:none; width: #{width}em;", :onchange => "$('#days_filter').submit();$(this).disabled();"}),
+        { :days  => select(nil, 'range', 1..number_of_days,
+                    {:selected => @range}, {:style=>"float:none; width: #{width}em;", :onchange =>"$('#days_filter').submit();$(this).disabled();"}),


Layout/SpaceAroundOperators: Surrounding space missing for operator =>.

houndci-bot · 2018-07-09T07:48:33Z

app/helpers/hosts_helper.rb

+      {:label=>_("Applied"), :data=>applied, :color =>'#89A54E'},
+      {:label=>_("Failed"), :data=>failed, :color =>'#AA4643'},
+      {:label=>_("Failed restarts"), :data=>failed_restarts, :color =>'#EC971F'},
+      {:label=>_("Skipped"), :data=>skipped, :color =>'#80699B'},


Layout/SpaceAroundOperators: Surrounding space missing for operator =>.

houndci-bot · 2018-07-09T07:48:33Z

app/helpers/hosts_helper.rb

+    {:results => [
+      {:label=>_("Applied"), :data=>applied, :color =>'#89A54E'},
+      {:label=>_("Failed"), :data=>failed, :color =>'#AA4643'},
+      {:label=>_("Failed restarts"), :data=>failed_restarts, :color =>'#EC971F'},


Layout/SpaceAroundOperators: Surrounding space missing for operator =>.

houndci-bot · 2018-07-09T07:48:34Z

app/helpers/hosts_helper.rb

-     {:label => _("Restarted"), :data => restarted, :color => '#4572A7'}]
+    {:results => [
+      {:label=>_("Applied"), :data=>applied, :color =>'#89A54E'},
+      {:label=>_("Failed"), :data=>failed, :color =>'#AA4643'},


Layout/SpaceAroundOperators: Surrounding space missing for operator =>.

houndci-bot · 2018-07-09T07:48:34Z

app/helpers/hosts_helper.rb

-     {:label => _("Skipped"), :data => skipped, :color => '#80699B'},
-     {:label => _("Restarted"), :data => restarted, :color => '#4572A7'}]
+    {:results => [
+      {:label=>_("Applied"), :data=>applied, :color =>'#89A54E'},


Layout/SpaceAroundOperators: Surrounding space missing for operator =>.

houndci-bot · 2018-07-09T07:48:34Z

app/helpers/hosts_helper.rb

+      failed          << [r.reported_at.to_i*1000, r.failed ]
+      restarted       << [r.reported_at.to_i*1000, r.restarted ]
+      failed_restarts << [r.reported_at.to_i*1000, r.failed_restarts ]
+      skipped         << [r.reported_at.to_i*1000, r.skipped ]


Layout/SpaceAroundOperators: Surrounding space missing for operator *.

houndci-bot · 2018-07-09T08:03:31Z

app/helpers/hosts_helper.rb

@@ -513,3 +518,4 @@ def host_breadcrumb
    breadcrumbs(resource_url: "/api/v2/hosts?thin=true'")
  end
 end
+


Layout/TrailingBlankLines: 1 trailing blank lines detected.

boaz0 · 2018-07-09T09:04:51Z

@ohadlevy the storybook can be found here

ohadlevy · 2018-07-09T09:53:40Z

there is an existing feature that is not ported, the current charts allow to zoom in, so one could select a time range and see a new chart... is that possible to enable that?

…

On Mon, Jul 9, 2018 at 12:04 PM ripcurld ***@***.***> wrote: @ohadlevy <https://github.com/ohadlevy> the storybook can be found here <https://rawgit.com/ripcurld/foreman/gh-pages/> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx68CJByeSyvpcR9oloyFD8CnRRgCks5uExy2gaJpZM4UIKXb> .

boaz0 · 2018-07-09T09:58:27Z

Unfortunately, I didn't find a way to do that in c3.js. You can zoom in and out but can't select a time-frame and zoom in.

boaz0 · 2018-09-18T10:21:05Z

@ohadlevy I had some free time, so I revisited c3.js documents and found how to enable brush selection.
It's a little bit different from the old one but it keeps the old behaviour the user was used to.
You can experiment it through the storybook here.

boaz0 · 2018-09-18T10:44:48Z

gr... katello dislikes me.

boaz0 · 2018-09-18T13:38:35Z

Lets hope pull --rebase from upstream HEAD will fix this.

boaz0 · 2018-09-19T22:23:51Z

[test katello]

boaz0 · 2018-09-19T23:45:58Z

[test foreman]

ohadlevy · 2018-10-03T13:57:45Z

@boaz1337 can you rebase please? thanks!

ohadlevy · 2018-10-03T14:00:37Z

@boaz1337 I just tried to use it (in the storybook) and I'm not sure how?

coveralls · 2018-10-04T08:59:49Z

Coverage increased (+0.7%) to 80.236% when pulling 408a38b on boaz1337:timeserieschart into d4e81e9 on theforeman:develop.

boaz0 · 2018-10-04T09:33:40Z

Uhm, I guess it's not as intuitive as I thought.
Anyway, the chart consists of two small charts:

top: the real chart that shows the data and displays a tooltip with the values. Also can be zoomed in/out by scrolling the wheel in the mouse
bottom: a chart that displays the big chart and the brush-selection. Selecting a specific area will render the chart above.

I hope this will demonstrate it better

boaz0 · 2018-10-04T13:29:00Z

CI failure is unrelated to changes.

ohadlevy · 2018-10-04T13:32:39Z

ping @theforeman/ui-ux ?

terezanovotna · 2018-10-05T08:18:50Z

this looks great! as it's pretty interactive, could I see a storybook to play with it? @boaz1337

boaz0 · 2018-10-05T08:24:21Z

@terezanovotna sure you can: storybook 📖

terezanovotna · 2018-10-05T08:44:36Z

looks good!

not sure how it's exactly applied, but when I open it for the first time, there is no grey rectangle.
Once I start playing with it, the rectangle appears and I get how to use it.

Can we have the grey rectangle on the screen by when opened for the first time?

The flot package is deprecated although it's being used to plot charts in Foreman. In this commit: - Added TimeseriesChart that uses Patternfly's LineChart under common/charts - Added HostChart under hosts - Rendered HostChart for runtime and resources in "hosts/show.html.erb" - Added unit tests - Removed unnecessary files such as "hosts/_runtime.html.erb" - Changed routes "hosts/:name/{runtime,resources}" to render JSON Signed-off-by: Boaz Shuster <[email protected]>

boaz0 · 2018-10-07T09:28:31Z

@terezanovotna sorry but right now there is no way to set the grey rectangle when chart is rendered for the first time.

EDIT looking into it more - in the new version of c3js you can do that by setting initialRange under zoom.

The problem is that bcbcarl/react-c3js#41 is still waiting on review.

ares · 2018-10-08T06:11:53Z

Sorry if I'm confused, but based on the storybook, this will be the way we displaye all charts? Do I have to activate the bottom chart somehow? The fact there are two charts for one, each for different time span would IMHO be quite confusing.

lizagilman · 2018-10-08T07:21:06Z

@boaz1337
Looks like this repository is barely maintained..

Rohoover · 2018-10-08T12:31:37Z

Sorry if I'm confused, but based on the storybook, this will be the way we displaye all charts? Do I have to activate the bottom chart somehow? The fact there are two charts for one, each for different time span would IMHO be quite confusing.

It starts out of with a view of the longest history. Activitating the bottom will allow a user to zoom in. Maybe not the most helpful or runtime but can be very beneficial if one would want to focus on the details of a specific section of chart.

I'm not seeing them being in different times? They are always representative of each other.

boaz0 · 2019-02-20T00:04:09Z

I'm closing this as we are stuck. The current UX is not good enough and the Chart library that PF3 is using is no longer up to date which makes it impossible to have something working.

If someone wants to take it, feel free to do it. I would love to help as much as I can.

Thanks everyone for your feedback and hard work. 😄

theforeman-bot added Needs testing Not yet reviewed labels May 22, 2018

ohadlevy suggested changes May 22, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels May 22, 2018

ohadlevy reviewed May 22, 2018

View reviewed changes

amirfefer suggested changes May 22, 2018

View reviewed changes

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 24, 2018

houndci-bot reviewed Jul 9, 2018

View reviewed changes

theforeman-bot added the UI label Sep 18, 2018

boaz0 closed this Feb 20, 2019

		import { noop } from '../../common/helpers';
		import Normalizer from './Normalizer';



		class TimeseriesChart extends React.Component {
		componentWillMount() {

Fixes #23665 - Use Patternfly charts in Runtime #5600

Fixes #23665 - Use Patternfly charts in Runtime #5600

Conversation

boaz0 commented May 22, 2018 • edited Loading

Before

After

theforeman-bot commented May 22, 2018

boaz0 commented May 22, 2018

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer May 22, 2018 • edited Loading

Choose a reason for hiding this comment

boaz0 commented May 30, 2018

ohadlevy commented May 30, 2018

boaz0 commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 commented Jul 9, 2018

ohadlevy commented Jul 9, 2018 via email

boaz0 commented Jul 9, 2018 • edited Loading

boaz0 commented Sep 18, 2018 • edited Loading

boaz0 commented Sep 18, 2018

boaz0 commented Sep 18, 2018

boaz0 commented Sep 19, 2018

boaz0 commented Sep 19, 2018

ohadlevy commented Oct 3, 2018

ohadlevy commented Oct 3, 2018

coveralls commented Oct 4, 2018

boaz0 commented Oct 4, 2018

boaz0 commented Oct 4, 2018

ohadlevy commented Oct 4, 2018

terezanovotna commented Oct 5, 2018

boaz0 commented Oct 5, 2018

terezanovotna commented Oct 5, 2018

boaz0 commented Oct 7, 2018 • edited Loading

ares commented Oct 8, 2018

lizagilman commented Oct 8, 2018

Rohoover commented Oct 8, 2018

boaz0 commented Feb 20, 2019

boaz0 commented May 22, 2018 •

edited

Loading

boaz0 May 22, 2018 •

edited

Loading

amirfefer May 22, 2018 •

edited

Loading

boaz0 commented Jul 9, 2018 •

edited

Loading

boaz0 commented Sep 18, 2018 •

edited

Loading

boaz0 commented Oct 7, 2018 •

edited

Loading