[FLINK-7812] Log system resources metrics #4801

pnowojski · 2017-10-11T12:54:16Z

What is the purpose of the change

This PR adds various system resources metrics, useful for analysing issues on machines/clusters for which there are no detailed external resources logging systems.

Verifying this change

This change was mostly manually tested, since it's difficult to write automated tests for this feature. However there is some trivial SystemResourcesCounterTest to make sure that codes executes without obvious mistakes.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)

It adds ohci dependency.

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

StephanEwen · 2017-10-13T10:24:32Z

Thanks for this addition. Few comments:

Please try to follow the common (though not enforced) code style when it comes to empty lines between class declarations, fields, methods, etc.
What is the license of the new dependency? Can you send a link so we can verify that?
If possible, let's shade that dependency, avoid increasing the dependency footprint.
Can we harmonize the config keys between the memory logger and the new system metrics?

pnowojski · 2017-10-13T11:54:26Z

As on dev mailing list discussion, this feature uses https://github.com/oshi/oshi library licensed under EPL 1.0. It seems to be compatible with ours: https://www.apache.org/legal/resolved.html .

It has minimal external dependencies. Question is whether we want to shade everything that we add?

Definitely we could unify config options if we want to do that.

StephanEwen · 2017-10-13T14:20:45Z

Eclipse Public License is not impossible, but tricky.

I am not a lawyer, but this is what I picked up over the year: EPL is weak copyleft, meaning linking is okay, but modifying not (from Apache License compatibility) . Shading the code in the library (which we do when building the flink dist jar) is a bit of an gray zone. It does not violate the spirit of the license, but a court may see that differently.

Various Flink users that approached us to avoid weak copyleft as much as possible because of that uncertainty, so avoiding this dependency would be desirable.

Making it an optional dependency that users explicitly have to add is possible, because then we do shade it into the Flink distribution jar.

pnowojski · 2017-10-27T07:26:06Z

I have made this dependency optional.

StephanEwen

Looks good, all in all.

I would ask to change all occurences of "additional logging" to "system resource logging", because "additional logging" is pretty non-descriptive.

There are also various places that are not checkstyle compliant, but not automatically caught, because the directory is currently checkstyle excluded. Please adjust them anyways, to make it easier to adopt more directories for checkstyle in the future.

StephanEwen · 2017-12-07T11:54:51Z

flink-runtime/src/test/java/org/apache/flink/runtime/taskexecutor/TaskManagerRunnerITCase.java

+/**
+ * Integration tests for proper initialization in the {@link TaskManagerRunner}.
+ */
+public class TaskManagerRunnerITCase {


This test seems very specific to this logging, but is named as a generic TaskManagerRunner test. Give it a differnet name?

Separate question: Does it have to be an IT case that fully starts the TM, or can it be a unit test that checks config propagation?

Renamed to SystemResourcesMetricsITCase.

I have added this test after some larger rebasing conflicts, because otherwise I wasn't sure that everything is starting up correctly. I think that there would have to be a handful unit tests replacing this one ITCase and they would be more prone to fail/cause problems during refactoring.

StephanEwen · 2017-12-07T12:07:03Z

...untime/src/main/java/org/apache/flink/runtime/taskexecutor/utils/SystemResourcesCounter.java

+ * CPU ticks and network sent/received bytes and then convert those values to CPU usage and
+ * send/receive byte rates.
+ */
+@ThreadSafe


A Thread is ThreadSafe? ;-)

Hmmm :) This annotation is about public getters, but maybe indeed it doesn't make a lot of sense. On the other hand, is it harmful?

StephanEwen · 2017-12-07T12:09:00Z

flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java

-			}
-		});
+		final NetworkBufferPool networkBufferPool = network.getNetworkBufferPool();
+		metrics.<Long, Gauge<Long>>gauge("TotalMemorySegments", () -> (long) networkBufferPool.getTotalNumberOfMemorySegments());


Replace with "Integer" Gauge and change to method reference?

aljoscha

I had one comment about settings naming but I can't really comment on the implementation. @zentol would be best for that

aljoscha · 2018-08-01T14:25:56Z

docs/_includes/generated/task_manager_configuration.html

@@ -37,6 +37,16 @@
            <td style="word-wrap: break-word;">true</td>
            <td>Enable SSL support for the taskmanager data transport. This is applicable only when the global ssl flag security.ssl.enabled is set to true</td>
        </tr>
+        <tr>
+            <td><h5>taskmanager.debug.additional-logging</h5></td>


How about changing these to system-resource-logging, as Stephan suggested?

nothing is actually being logger as far as i can tell, so I would suggest system-resource-metrics and system-resource-metrics-polling-interval.

I renamed them to: system-resource-metrics and system-resource-metrics-probing-interval. polling didn't seem quite appropriate.

zentol

In several places we refer to "logging" additional metrics which seems misleading, as nothing is actually logged.

Why are we restricting this to taskmanagers? I'm reasonably certain that any user that enables this for TMs would like to enable this for all containers for consistency alone.

zentol · 2018-08-01T14:54:20Z

docs/monitoring/metrics.md

+resources metrics are updated periodically and they present average values for a configured
+interval (`taskmanager.debug.additional-logging-interval`).
+
+Logging of system resources require couple of optional dependencies to be present on the


"requires a"

zentol · 2018-08-01T15:01:38Z

docs/_includes/generated/task_manager_configuration.html

@@ -37,6 +37,16 @@
            <td style="word-wrap: break-word;">true</td>
            <td>Enable SSL support for the taskmanager data transport. This is applicable only when the global ssl flag security.ssl.enabled is set to true</td>
        </tr>
+        <tr>
+            <td><h5>taskmanager.debug.additional-logging</h5></td>


nothing is actually being logger as far as i can tell, so I would suggest system-resource-metrics and system-resource-metrics-polling-interval.

zentol · 2018-08-01T15:02:43Z

...untime/src/main/java/org/apache/flink/runtime/taskexecutor/utils/SystemResourcesCounter.java

+import static org.apache.flink.util.Preconditions.checkState;
+
+/**
+ * Daemon thread logging system resources.


outdated, nothing is being logged.

Changed to Daemon thread probing system resources.

zentol · 2018-08-01T15:02:55Z

...main/java/org/apache/flink/runtime/taskexecutor/utils/SystemResourcesMetricsInitializer.java

+import oshi.hardware.HardwareAbstractionLayer;
+
+/**
+ * Utility class ot initialize system resources metrics.


"to initialize system resource"

zentol · 2018-08-01T15:03:33Z

...main/java/org/apache/flink/runtime/taskexecutor/utils/SystemResourcesMetricsInitializer.java

+		}
+		catch (NoClassDefFoundError ex) {
+			LOG.warn(
+				"Failed to initialize system resources metrics because of missing classes definitions." +


"system resource metrics" "missing class definitions"

zentol · 2018-08-01T15:04:14Z

flink-runtime/src/main/scala/org/apache/flink/runtime/taskmanager/TaskManager.scala

@@ -1884,13 +1884,23 @@ object TaskManager {

      // if desired, start the logging daemon that periodically logs the
      // memory usage information
-      if (LOG.isInfoEnabled && configuration.getBoolean(


these changes shouldn't be necessary anymore.

zentol · 2018-08-01T15:08:38Z

Also needs a rebase.

zentol · 2018-08-01T15:10:09Z

docs/monitoring/metrics.md

+classpath (for example placed in Flink's `lib` directory):
+
+  - `com.github.oshi:oshi-core:3.4.0` (licensed under EPL 1.0 license)
+  - `net.java.dev.jna:jna-platform:jar:4.2.2`


are these transitive dependency of oshi-core?

yes. I have rephrased this paragraph.

zentol · 2018-08-01T15:10:57Z

...untime/src/main/java/org/apache/flink/runtime/taskexecutor/utils/SystemResourcesCounter.java

+			}
+		} catch (InterruptedException e) {
+			if (running) {
+				LOG.error("{} has failed", SystemResourcesCounter.class.getSimpleName(), e);


log as warning instead

pnowojski

Thanks for the review. Applied requested changes.

pnowojski · 2018-08-02T08:55:41Z

docs/_includes/generated/task_manager_configuration.html

@@ -37,6 +37,16 @@
            <td style="word-wrap: break-word;">true</td>
            <td>Enable SSL support for the taskmanager data transport. This is applicable only when the global ssl flag security.ssl.enabled is set to true</td>
        </tr>
+        <tr>
+            <td><h5>taskmanager.debug.additional-logging</h5></td>


I renamed them to: system-resource-metrics and system-resource-metrics-probing-interval. polling didn't seem quite appropriate.

pnowojski · 2018-08-02T09:01:47Z

docs/monitoring/metrics.md

+classpath (for example placed in Flink's `lib` directory):
+
+  - `com.github.oshi:oshi-core:3.4.0` (licensed under EPL 1.0 license)
+  - `net.java.dev.jna:jna-platform:jar:4.2.2`


yes. I have rephrased this paragraph.

pnowojski · 2018-08-02T09:02:44Z

...untime/src/main/java/org/apache/flink/runtime/taskexecutor/utils/SystemResourcesCounter.java

+import static org.apache.flink.util.Preconditions.checkState;
+
+/**
+ * Daemon thread logging system resources.


Changed to Daemon thread probing system resources.

pnowojski · 2018-08-02T12:38:32Z

@zentol rebased and added system resource metrics both to job manager and task manager.

zentol · 2018-08-02T13:38:40Z

docs/_includes/generated/core_configuration.html

@@ -22,6 +22,16 @@
            <td style="word-wrap: break-word;">"child-first"</td>
            <td>Defines the class resolution strategy when loading classes from user code, meaning whether to first check the user code jar ("child-first") or the application classpath ("parent-first"). The default settings indicate to load classes first from the user code jar, which means that user code jars can include and load different dependencies than Flink uses (transitively).</td>
        </tr>
+        <tr>
+            <td><h5>debug.system-resource-metrics</h5></td>


How about moving these to MetricOptions? (they could also have a "metrics" prefix instead)

zentol · 2018-08-02T13:39:10Z

docs/monitoring/metrics.md

-resources metrics are updated periodically and they present average values for a configured
-interval (`taskmanager.debug.additional-logging-interval`).
+System resources reporting is disabled by default. When `debug.system-resource-metrics`
+is enabled additional metrics listed below will be available on TaskManager and JobManager.


"on Job- and TaskManagers"

zentol · 2018-08-02T13:42:12Z

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

@@ -2517,9 +2517,11 @@ object JobManager {
      }
    }

+


remove empty line

zentol · 2018-08-02T13:44:05Z

...-tests/src/test/java/org/apache/flink/runtime/taskexecutor/SystemResourcesMetricsITCase.java

 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;

 /**
- * Integration tests for proper initialization of the system resource metrics in the {@link TaskManagerRunner}.
+ * Integration tests for proper initialization of the system resource metrics.


This test should be updated to check that they are reported for both JM and TM.

zentol · 2018-08-02T13:47:08Z

...me/src/test/java/org/apache/flink/runtime/taskexecutor/utils/SystemResourcesCounterTest.java

+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.taskexecutor.utils;


move to "runtime.metrics"

zentol · 2018-08-02T13:47:35Z

...-tests/src/test/java/org/apache/flink/runtime/taskexecutor/SystemResourcesMetricsITCase.java

+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.taskexecutor;


move to "runtime.metrics"

zentol · 2018-08-02T13:48:26Z

...-tests/src/test/java/org/apache/flink/runtime/taskexecutor/SystemResourcesMetricsITCase.java

+
+	@Test
+	public void startTaskManagerAndCheckForRegisteredSystemMetrics() throws Exception {
+		assertEquals(1, TestReporter.OPENED_REPORTERS.size());


there should be 2 reporters (it should fail in the current state)

There should be one. MiniCluster is starting up both job manager and task manager reusing the same reporter. Their metrics are only prefixed with localhost.jobmanager. and localhost.taskamanger.TASKMANAGER_ID.

Added test coverage to look for both jobmanager and taskmanager metrics.

zentol · 2018-08-03T15:26:58Z

+1

pnowojski · 2018-08-06T11:27:40Z

Thanks for the review @zentol ! Merging :)

This closes apache#4801.

This closes apache#4801. (cherry picked from commit cd880fb) (cherry picked from commit 80e6b77)

pnowojski force-pushed the resources branch 3 times, most recently from 776bb1d to 1dbafb7 Compare October 12, 2017 09:11

pnowojski force-pushed the resources branch from c90f25f to a352a57 Compare October 16, 2017 10:22

pnowojski force-pushed the resources branch 2 times, most recently from 04a1d30 to 0aa27fb Compare November 28, 2017 09:18

StephanEwen requested changes Dec 7, 2017

View reviewed changes

pnowojski force-pushed the resources branch 2 times, most recently from 4f791c0 to aae077c Compare December 11, 2017 10:27

pnowojski force-pushed the resources branch 3 times, most recently from 292d6ab to d4f48fa Compare June 29, 2018 12:59

aljoscha reviewed Aug 1, 2018

View reviewed changes

zentol requested changes Aug 1, 2018

View reviewed changes

zentol reviewed Aug 1, 2018

View reviewed changes

pnowojski commented Aug 2, 2018

View reviewed changes

pnowojski force-pushed the resources branch from d4f48fa to 0eb3680 Compare August 2, 2018 12:38

pnowojski force-pushed the resources branch 2 times, most recently from a279b1b to 2ab886d Compare August 2, 2018 13:01

zentol requested changes Aug 2, 2018

View reviewed changes

pnowojski force-pushed the resources branch from 19ce6ac to a9f6e66 Compare August 3, 2018 11:04

pnowojski added 2 commits August 6, 2018 13:29

[hotfix][metrics] Replace anonymous classes with lambdas

ed58c34

[FLINK-7812][metrics] Add system resources metrics

cd880fb

This closes apache#4801.

pnowojski force-pushed the resources branch from a9f6e66 to cd880fb Compare August 6, 2018 11:40

asfgit merged commit cd880fb into apache:master Aug 6, 2018

NicoK pushed a commit to dataArtisans/flink that referenced this pull request Aug 8, 2018

[FLINK-7812][metrics] Add system resources metrics

83d1bb9

This closes apache#4801.

NicoK pushed a commit to dataArtisans/flink that referenced this pull request Sep 18, 2018

[FLINK-7812][metrics] Add system resources metrics

892fffb

This closes apache#4801.

Clarkkkkk pushed a commit to Clarkkkkk/flink that referenced this pull request Nov 29, 2018

[FLINK-7812][metrics] Add system resources metrics

9456fab

This closes apache#4801. (cherry picked from commit cd880fb) (cherry picked from commit 80e6b77)

rmetzger added the component=Runtime/Metrics label Mar 18, 2019

[FLINK-7812] Log system resources metrics #4801

[FLINK-7812] Log system resources metrics #4801

Conversation

pnowojski commented Oct 11, 2017 • edited

What is the purpose of the change

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

StephanEwen commented Oct 13, 2017

pnowojski commented Oct 13, 2017

StephanEwen commented Oct 13, 2017

pnowojski commented Oct 27, 2017

StephanEwen 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

aljoscha 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

zentol 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

Choose a reason for hiding this comment

zentol commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowojski 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

pnowojski commented Aug 2, 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

pnowojski Aug 3, 2018 • edited

Choose a reason for hiding this comment

zentol commented Aug 3, 2018

pnowojski commented Aug 6, 2018

pnowojski commented Oct 11, 2017 •

edited

pnowojski Aug 3, 2018 •

edited