[ALLUXIO-2182] Added a metrics master for aggregating the client metrics #7251

yupeng9 · 2018-05-07T21:13:49Z

https://alluxio.atlassian.net/browse/ALLUXIO-2182

Each client sends over the metrics to the master in its heartbeat. And the new metrics master will aggregate the metrics defined by the aggregators.

This PR adds a client-master heartbeat to send the client-side metrics info. More test coverage improvement will be added once we agree on the architecture.

…nt-metr

AmplabJenkins · 2018-05-07T21:28:08Z

Merged build finished. Test FAILed.

aaudiber

High-level review:

We might lose metrics from short-lived clients because they exit before sending the final heartbeat to master. Do you think it makes sense to send one last metric heartbeat before letting the JVM exit?
It could be useful if to make the client ID configurable so that users can use the same ID across their application and then aggregate to see only metrics for their application.
Could we add a metric to track how many master threads are being consumed by client metric heartbeats?

yupeng9 · 2018-05-07T22:28:26Z

#1 We could add a shutdown hook for the last heartbeat. However, I feel this is a bit overengineered, as the metrics, imo, might not necessary to be exact but more best efforts. Losing some metrics in 1-2s might not be too problematic.
#2 We can have it as a configurable value, but before we see some real use cases or requests, I feel it is not a must-have feature to be in this framework PR.
#3 We could. But I want to ramp up metrics in future PRs, and this one is to set up the foundation.

calvinjia · 2018-05-07T23:21:51Z

@yupeng9 What's the overhead of the client to master heartbeat, could this be a problem in environments with 1000s of clients?

aaudiber · 2018-05-07T23:29:10Z

If the client runs for shorter than the heartbeat interval, it will send no metrics. I think that could be problematic, especially if we need to increase the heartbeat interval to reduce pressure on the master.
It seems like a very low-hanging fruit that's much more useful than per-jvm randomly generated ids. We can do it in a different PR if you prefer, but the ID used to identify clients seems like it fits in this PR as a part of the foundation/framework PR for client metrics.
Ok, future PR is fine

yupeng9 · 2018-05-08T05:15:51Z

@aaudiber
hmm, for 1, I'm not that concerned, because the number of metrics that can happen within 1-2s should not be large. But I'm okay with adding it if you insist.

For 2, I prefer a separate PR for adding this optional client id.

yupeng9 · 2018-05-08T05:20:02Z

@calvinjia that's why I make the frequency of heartbeat configurable. The metrics collection, IMO, is not necessary to be too frequent given its nature. For 1 thousand service handling every a few seconds should be very doable. However, it does take connections to serve the heartbeat request, so increasing the size of thrift thread pool might be needed.

yupeng9 · 2018-05-08T05:20:52Z

@calvinjia in addition, I can put this feature behind a flag, so for large-scale applications, metrics can be turned off if it affects performance.

calvinjia · 2018-05-08T15:26:19Z

@yupeng9 Have you verified there are threadpool issues when running with a large number of clients and does increasing the threadpool solve the issue without any side effects? I think it is important to have a solution which works for larger clusters (as opposed to just turning it off).

AmplabJenkins · 2018-05-08T17:23:22Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-08T22:22:12Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-08T22:22:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/19531/
Test FAILed.

AmplabJenkins · 2018-05-08T23:23:24Z

Merged build finished. Test FAILed.

calvinjia · 2018-05-10T00:55:12Z

core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java

@@ -133,6 +149,9 @@ public static FileSystemContext create(Subject subject, MasterInquireClient mast
   */
  private FileSystemContext(Subject subject) {
    mParentSubject = subject;
+    mExecutorService = Executors.newFixedThreadPool(1,
+        ThreadFactoryUtils.build("metrics-master-heartbeat-%d", true));
+    mId = UUID.randomUUID().toString().replace("-", "");


Should this be an IdUtils method?

calvinjia · 2018-05-10T01:05:24Z

core/client/fs/src/main/java/alluxio/client/metrics/ClientMasterSync.java

+ * If the task fails to heartbeat to the master, it will destroy its old master client and recreate
+ * it before retrying.
+ */
+@NotThreadSafe


Don't we access this from multiple threads despite it being not thread safe?

This is a heartbeat executor, and it's only invoked once per JVM?

Don't we also access it in the shutdown hook?

That's true.

calvinjia · 2018-05-10T01:06:04Z

core/client/fs/src/main/java/alluxio/client/metrics/ClientMasterSync.java

+   */
+  public ClientMasterSync(MetricsMasterClient masterClient) {
+    mMasterClient = Preconditions.checkNotNull(masterClient, "masterClient");
+    mHeartbeatTimeoutMs = (int) Configuration.getMs(PropertyKey.WORKER_BLOCK_HEARTBEAT_TIMEOUT_MS);


This timeout value seems unrelated?

calvinjia · 2018-05-10T01:07:34Z

core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java

+   * @return the unique id of the context
+   */
+  public String getId() {
+    return mId;


Is there any way to determine the ID of a process, how will users be able to map an ID to a process after seeing the metrics be abnormal?

I considered this, but there does not seem a good approach commonly agreed on: https://stackoverflow.com/questions/35842/how-can-a-java-program-get-its-own-process-id?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

We could log the ID after it's created.

Logging sounds good for now.

calvinjia · 2018-05-10T01:10:53Z

core/client/fs/src/main/java/alluxio/client/metrics/ClientMasterSync.java

+            throw new RuntimeException("Master heartbeat timeout exceeded: " + mHeartbeatTimeoutMs);
+          }
+          LOG.error("Master heartbeat timeout exceeded: " + mHeartbeatTimeoutMs);
+          System.exit(-1);


Do we really want to system exit if the metrics heartbeat times out?

Removed and made it best effort to send the metrics info.

calvinjia · 2018-05-10T01:19:49Z

core/common/src/main/java/alluxio/heartbeat/HeartbeatContext.java

@@ -40,6 +40,7 @@
  public static final String WORKER_FILESYSTEM_MASTER_SYNC = "Worker FileSystemMaster Sync";
  public static final String WORKER_PIN_LIST_SYNC = "Worker Pin List Sync";
  public static final String WORKER_SPACE_RESERVER = "Worker Space Reserver";
+  public static final String METRICS_MASTER_SYNC = "Metrics Master Sync";


Perhaps name this similar to the others Master ...?

calvinjia · 2018-05-10T01:20:25Z

core/common/src/main/java/alluxio/metrics/ClientMetrics.java

+public final class ClientMetrics {
+  /**
+   * Total number of bytes short-circuit read from local storage.
+   */


/** ... */ format

calvinjia · 2018-05-10T01:22:53Z

core/common/src/thrift/common.thrift

-  4: i64 value
+  3: string instanceId
+  4: string name
+  5: i64 value


This change is backward incompatible?

Metric is added in this branch, so it's not public yet

Got it, thanks

calvinjia · 2018-05-10T01:24:29Z

core/server/master/src/main/java/alluxio/master/metrics/DefaultMetricsMaster.java

+  public void clientHeartbeat(String clientId, String hostname, List<Metric> metrics) {
+    if (metrics.isEmpty()) {
+      return;
+    }


Could we do this logic in putClientMetrics, since it should be able to handle an empty set?

calvinjia · 2018-05-10T01:26:19Z

core/server/master/src/main/java/alluxio/master/metrics/MetricsMaster.java

+   * @param hostname the client hostname
+   * @param metrics client-side metrics
+   */
+  void clientHeartbeat(String clientId, String hostname, List<Metric> metrics);


Since clientId is in metrics, we could make this one API. If we keep multiple APIs, could we name them symmetrically?

What do you mean? This API only handles heartbeat from the client, while the worker heartbeats are handled in the BlockMaster.

I meant the clientHeartbeat and putWorkerMetrics methods

AmplabJenkins · 2018-05-14T18:54:56Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-14T21:01:29Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-15T02:31:33Z

Merged build finished. Test FAILed.

alluxio-bot · 2018-05-18T20:56:29Z

Automated checks report:

Valid pull request title: PASS
Contains link to JIRA ticket: PASS
Commits associated with Github account: PASS

All checks passed!

alluxio-bot · 2018-05-18T21:38:07Z

Automated checks report:

Valid pull request title: PASS
Contains link to JIRA ticket: PASS
Commits associated with Github account: PASS

All checks passed!

AmplabJenkins · 2018-05-18T21:42:59Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-18T21:43:00Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/19695/
Test FAILed.

AmplabJenkins · 2018-05-18T22:06:44Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-18T22:06:44Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/19690/

Failed Tests: 17

org.alluxio:alluxio-tests: 17

Test FAILed.

alluxio-bot · 2018-05-18T22:31:02Z

Automated checks report:

Valid pull request title: PASS
Contains link to JIRA ticket: PASS
Commits associated with Github account: PASS

All checks passed!

aaudiber

LGTM

AmplabJenkins · 2018-05-18T23:01:12Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-18T23:01:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/19696/

Failed Tests: 17

org.alluxio:alluxio-tests: 17

Test FAILed.

AmplabJenkins · 2018-05-18T23:37:05Z

Merged build finished. Test PASSed.

AmplabJenkins · 2018-05-18T23:37:06Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/19697/
Test PASSed.

alluxio-bot · 2018-05-18T23:37:09Z

Automated checks report:

Valid pull request title: PASS
Contains link to JIRA ticket: PASS
Commits associated with Github account: PASS

All checks passed!

yupeng9 added 3 commits May 2, 2018 16:52

add client-side metrics collection

b359378

Merge remote-tracking branch 'upstream/metrics-enhancement' into clie…

4f14444

…nt-metr

checkstyle

4397139

yupeng9 requested review from calvinjia and aaudiber May 7, 2018 21:13

aaudiber suggested changes May 7, 2018

View reviewed changes

add shutdown hook

905ca2a

fix

e42602b

stylecheck

b396eda

calvinjia requested changes May 10, 2018

View reviewed changes

yupeng9 added 3 commits May 14, 2018 10:40

comments

361d01a

checkstyle

f199883

add tests for metrics store

ff3508f

add more tests

8b5b38d

comments

e475015

Alluxio deleted a comment from AmplabJenkins May 18, 2018

comment

8db4f52

comments

d179e98

findbug

6c9c32b

calvinjia approved these changes May 18, 2018

View reviewed changes

fix test

4eb4320

aaudiber approved these changes May 18, 2018

View reviewed changes

yupeng9 merged commit 7360bb0 into Alluxio:metrics-enhancement May 18, 2018

[ALLUXIO-2182] Added a metrics master for aggregating the client metrics #7251

[ALLUXIO-2182] Added a metrics master for aggregating the client metrics #7251

Conversation

yupeng9 commented May 7, 2018 • edited Loading

AmplabJenkins commented May 7, 2018

aaudiber left a comment

Choose a reason for hiding this comment

yupeng9 commented May 7, 2018

calvinjia commented May 7, 2018

aaudiber commented May 7, 2018

yupeng9 commented May 8, 2018

yupeng9 commented May 8, 2018

yupeng9 commented May 8, 2018

calvinjia commented May 8, 2018

AmplabJenkins commented May 8, 2018

AmplabJenkins commented May 8, 2018

AmplabJenkins commented May 8, 2018

AmplabJenkins commented May 8, 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

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

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

AmplabJenkins commented May 14, 2018

AmplabJenkins commented May 14, 2018

AmplabJenkins commented May 15, 2018

alluxio-bot commented May 18, 2018

alluxio-bot commented May 18, 2018

AmplabJenkins commented May 18, 2018

AmplabJenkins commented May 18, 2018

AmplabJenkins commented May 18, 2018

AmplabJenkins commented May 18, 2018

Failed Tests: 17

org.alluxio:alluxio-tests: 17

alluxio-bot commented May 18, 2018

aaudiber left a comment

Choose a reason for hiding this comment

AmplabJenkins commented May 18, 2018

AmplabJenkins commented May 18, 2018

Failed Tests: 17

org.alluxio:alluxio-tests: 17

AmplabJenkins commented May 18, 2018

AmplabJenkins commented May 18, 2018

alluxio-bot commented May 18, 2018

yupeng9 commented May 7, 2018 •

edited

Loading