[GitHub] [gobblin] autumnust commented on a change in pull request #3365: [GOBBLIN-1516] Only schedule TaskMetricsUpdateder when metric-reporting is enabled by config

[GitBox <gi...@apache.org>]

autumnust commented on a change in pull request #3365:
URL: https://github.com/apache/gobblin/pull/3365#discussion_r689834209



##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTaskStateTracker.java
##########
@@ -61,7 +61,9 @@ public GobblinHelixTaskStateTracker(Properties properties) {
   @Override
   public void registerNewTask(Task task) {
     try {
-      this.scheduledReporters.put(task.getTaskId(), scheduleTaskMetricsUpdater(new TaskMetricsUpdater(task), task));
+      if (GobblinMetrics.isEnabled(task.getTaskState().getWorkunit())) {
+        this.scheduledReporters.put(task.getTaskId(), scheduleTaskMetricsUpdater(new TaskMetricsUpdater(task), task));

Review comment:
       the additional thing being placed within the condition of `metrics.enabled` to be true, a.k.a `TaskMetricsUpdater` seems to be pretty metrics related and lightweight as it calls `updateRecordMetrics` and `updateByteMetrics`. Both of them are included if instrumented writer is being used, so I believe it is fair to gate these behavior with this config. 
   
   If it is reversed, i.e. the `metrics.enabled` is controlling lightweight metrics-reporting behavior while we are trying to let it control the emission of GTE then I will be concerned as well. Let me know if it makes sense. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org