[FLINK-2730][web-dashboard] Adds cpu and memory usage graphs. #1236

sachingoel0101 · 2015-10-07T11:54:50Z

Screenshots:

I would really like to get this in the release. One more step towards removing the old frontend. :)

aljoscha · 2015-10-07T13:10:02Z

Wow, the screenshots look super nice. 😄

Someone with knowledge about the new web fronted should probably review the code, though.

tillrohrmann · 2015-10-07T13:17:29Z

Really cool graphs Sachin :-) Looking forward monitoring my Flink jobs with
these graphs.

On Wed, Oct 7, 2015 at 3:10 PM, Aljoscha Krettek notifications@github.com
wrote:

Wow, the screenshots look super nice. [image: 😄]

Someone with knowledge about the new web fronted should probably review
the code, though.

—
Reply to this email directly or view it on GitHub
#1236 (comment).

sachingoel0101 · 2015-10-07T13:17:43Z

Thanks. :)
Although there's not much to review here. :') It's mostly a rearrangement of existing content, with a major addition in taskmanager.ctrl.coffee

aljoscha · 2015-10-07T13:46:38Z

@iampeter, could you please have a look so that we can quickly merge this?

iampeter · 2015-10-07T20:37:23Z

@aljoscha ok will do

iampeter · 2015-10-07T21:23:28Z

@sachingoel0101 @aljoscha looks great, but I would make the following changes if possible:

Move the 'livechart' directive to a separate file - taskmanager.dir.coffee
Move the Highcharts config in index.coffee to either .run() in index.coffee, or the directive
Remove the 'No such task manager exists' text that blinks when the task manager details are loading

Thanks,
Piotr

aljoscha · 2015-10-08T06:56:20Z

Thanks for the review @iampeter 😄

sachingoel0101 · 2015-10-08T07:02:57Z

@iampeter , thanks for the review. I've addressed all your comments.
I removed the No such task ... thing entirely. It doesn't have a use case anyways. We don't expect the user to break things on the server here. If the task manager doesn't exist, it is handled gracefully by the backend and an empty array is returned.

sachingoel0101 · 2015-10-08T07:05:28Z

@aljoscha FYI I've added a new Job Manager message which returns the metrics only for one particular instance to minimize unnecessary traffic for a single taskmanager web page. That okay?

sachingoel0101 · 2015-10-08T07:10:25Z

I think I broke something while addressing Piotr's concerns. Will debug and push the fixed version again.
Edit: Never mind. False alarm. 😄

Should be mergeable now.

mxm · 2015-10-09T08:50:58Z

The screenshots look great. I'll check out your work now :)

uce · 2015-10-09T09:32:50Z

...time-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/TaskManagersHandler.java

@@ -39,6 +42,7 @@

 	private final FiniteDuration timeout;

+	private static final String taskManagerIDKey = "taskmanagerid";


This is referencing your addition in WebRuntimeMonitor. You can use this constant there as well. I would furthermore use the constant field style a la "TASKMANAGER_ID_KEY".

Pushed a fix.

mxm · 2015-10-09T10:40:58Z

flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala

+      sender ! decorateMessage(
+        TaskManagerInstance(instanceManager.getRegisteredInstanceById(instanceID))
+      )
+


Wouldn't it be better to return an InstanceNotFound message instead of a null instance?

I think using an Option makes more sense here.

mxm · 2015-10-09T10:43:52Z

Tried out your changes it everything worked without quirks. The initial look is a bit odd when the charts haven't loaded yet but this is nothing that should prevent us from merging the PR. I've also made a comment inline.

+1 for merging

sachingoel0101 · 2015-10-09T10:53:00Z

The initial look is a bit weird, I agree. There is a potential fix, by fetching the usage upto the point when someone clicks on the metrics tab, however, that can be quite large if the cluster has been running for a long time. This would also require storing the metrics history on the job manager, which will unnecessarily waste memory.

sachingoel0101 · 2015-10-09T11:45:57Z

@mxm I modified the code to remove null values. It should be mergeable now.

mxm · 2015-10-09T13:27:24Z

Looks good to merge now.

sachingoel0101 · 2015-10-09T16:18:32Z

Build passes successfully. Looking forward to seeing this merged. :)

mxm · 2015-10-09T16:29:49Z

Merging this after a final test...

This closes apache#1236.

mxm · 2015-11-09T13:36:18Z

flink-runtime-web/web-dashboard/bower.json

@@ -23,7 +23,8 @@
    "dagre-d3": "~0.4.10",
    "font-awesome": "~4.3.0",
    "moment-duration-format": "~1.3.0",
-    "qtip2": "~2.2.1"
+    "qtip2": "~2.2.1",
+    "highcharts-release": "~4.1.8"


This library's license is incompatible with the Apache License. Unfortunately, nobody noticed this. We will have to revert the merged changes...

I apologize. It should've been my responsibility primarily.
I think I might be able to achieve the same graphs and live updates using d3, which I assume is compatible. Working on it right now.

Thanks! For now, we'll try to make a release without that page. We can add that back for 0.10.1 and 1.0

Okay. I can a open a PR to minimally remove this functionality unless the commit needs to be completely reverted.

I'm already working on a change to remove only the charts. Apparently, d3 would work for us because it is BSD-lincensed.

Great. I'll start working on d3 based charts after you push your commit.

sachingoel0101 force-pushed the charts branch 3 times, most recently from 81b7f5b to 46ca697 Compare October 7, 2015 16:26

sachingoel0101 force-pushed the charts branch from 46ca697 to ce56950 Compare October 8, 2015 06:42

uce reviewed Oct 9, 2015
View reviewed changes

sachingoel0101 force-pushed the charts branch from ce56950 to a5cdfb1 Compare October 9, 2015 10:01

mxm reviewed Oct 9, 2015
View reviewed changes

[FLINK-2730][web-dashboard] Adds cpu and memory usage graphs.

338d42d

sachingoel0101 force-pushed the charts branch from a5cdfb1 to 338d42d Compare October 9, 2015 11:43

asfgit closed this in 58ab14b Oct 9, 2015

sachingoel0101 deleted the charts branch October 9, 2015 21:30

cfmcgrady pushed a commit to cfmcgrady/flink that referenced this pull request Oct 23, 2015

[FLINK-2730][webfrontend] Add cpu and memory usage graphs

4eeb461

This closes apache#1236.

lofifnc pushed a commit to lofifnc/flink that referenced this pull request Oct 23, 2015

[FLINK-2730][webfrontend] Add cpu and memory usage graphs

77969ec

This closes apache#1236.

mxm reviewed Nov 9, 2015
View reviewed changes

rmetzger added the component=Runtime/WebFrontend label Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-2730][web-dashboard] Adds cpu and memory usage graphs. #1236

[FLINK-2730][web-dashboard] Adds cpu and memory usage graphs. #1236

sachingoel0101 commented Oct 7, 2015

aljoscha commented Oct 7, 2015

tillrohrmann commented Oct 7, 2015

sachingoel0101 commented Oct 7, 2015

aljoscha commented Oct 7, 2015

iampeter commented Oct 7, 2015

iampeter commented Oct 7, 2015

aljoscha commented Oct 8, 2015

sachingoel0101 commented Oct 8, 2015

sachingoel0101 commented Oct 8, 2015

sachingoel0101 commented Oct 8, 2015

mxm commented Oct 9, 2015

uce Oct 9, 2015

sachingoel0101 Oct 9, 2015

mxm Oct 9, 2015

sachingoel0101 Oct 9, 2015

mxm commented Oct 9, 2015

sachingoel0101 commented Oct 9, 2015

sachingoel0101 commented Oct 9, 2015

mxm commented Oct 9, 2015

sachingoel0101 commented Oct 9, 2015

mxm commented Oct 9, 2015

mxm Nov 9, 2015

sachingoel0101 Nov 9, 2015

StephanEwen Nov 9, 2015

sachingoel0101 Nov 9, 2015

mxm Nov 9, 2015

sachingoel0101 Nov 9, 2015

		@@ -39,6 +42,7 @@

		private final FiniteDuration timeout;

		private static final String taskManagerIDKey = "taskmanagerid";

[FLINK-2730][web-dashboard] Adds cpu and memory usage graphs. #1236

[FLINK-2730][web-dashboard] Adds cpu and memory usage graphs. #1236

Conversation

sachingoel0101 commented Oct 7, 2015

aljoscha commented Oct 7, 2015

tillrohrmann commented Oct 7, 2015

sachingoel0101 commented Oct 7, 2015

aljoscha commented Oct 7, 2015

iampeter commented Oct 7, 2015

iampeter commented Oct 7, 2015

aljoscha commented Oct 8, 2015

sachingoel0101 commented Oct 8, 2015

sachingoel0101 commented Oct 8, 2015

sachingoel0101 commented Oct 8, 2015

mxm commented Oct 9, 2015

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

mxm commented Oct 9, 2015

sachingoel0101 commented Oct 9, 2015

sachingoel0101 commented Oct 9, 2015

mxm commented Oct 9, 2015

sachingoel0101 commented Oct 9, 2015

mxm commented Oct 9, 2015

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