You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2022/05/04 17:26:40 UTC

[GitHub] [gobblin] Will-Lo opened a new pull request, #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Will-Lo opened a new pull request, #3500:
URL: https://github.com/apache/gobblin/pull/3500

   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1639
   
   
   ### Description
   - [x] Here are some details about my PR, including screenshots (if applicable):
   
   For Adhoc flows in GaaS, metrics are still being emitted on the Gobblin pipeline. This will use the adhoc configuration to determine whether or not to emit these metrics.
   
   Also a small improvement to the workunit count metric to be emitted on the gobblin pipeline. In the future, all pipeline related metrics should be only emitted by the pipeline, not by the service.
   
   ### Tests
   - [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   
   
   ### Commits
   - [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   


-- 
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

[GitHub] [gobblin] ZihanLi58 merged pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
ZihanLi58 merged PR #3500:
URL: https://github.com/apache/gobblin/pull/3500


-- 
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

[GitHub] [gobblin] umustafi commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r866144603


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,8 +674,23 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed
       if (this.jobContext.getJobMetricsOptional().isPresent()) {
+        if (jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+          String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,
+              TimingEvent.LauncherTimings.WORK_UNITS_CREATION, jobState.getJobName());
+          long workUnitsCreationTime = workUnitsCreationTimer.getDuration() / TimeUnit.SECONDS.toMillis(1);

Review Comment:
   I agree that we should always emit a workunit count because this is most useful when the workflow does _not_ perform work. Wrapping it in a try can make this safer to avoid throwing an exception. 



-- 
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

[GitHub] [gobblin] codecov-commenter commented on pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#issuecomment-1117626244

   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3500?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3500](https://codecov.io/gh/apache/gobblin/pull/3500?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7e51d33) into [master](https://codecov.io/gh/apache/gobblin/commit/48b25a2fde983608250c21d5dfabf50b81a7a0d0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (48b25a2) will **decrease** coverage by `3.19%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3500      +/-   ##
   ============================================
   - Coverage     46.67%   43.48%   -3.20%     
   + Complexity    10405     2040    -8365     
   ============================================
     Files          2078      405    -1673     
     Lines         81289    17454   -63835     
     Branches       9078     2133    -6945     
   ============================================
   - Hits          37945     7590   -30355     
   + Misses        39846     9022   -30824     
   + Partials       3498      842    -2656     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3500?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFzay5qYXZh) | `60.21% <0.00%> (-2.16%)` | :arrow_down: |
   | [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | | |
   | [...rg/apache/gobblin/runtime/AbstractJobLauncher.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvQWJzdHJhY3RKb2JMYXVuY2hlci5qYXZh) | | |
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | | |
   | [...service/modules/orchestration/DagManagerUtils.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXJVdGlscy5qYXZh) | | |
   | [...gobblin/service/modules/spec/JobExecutionPlan.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zcGVjL0pvYkV4ZWN1dGlvblBsYW4uamF2YQ==) | | |
   | [...blin/service/monitoring/KafkaJobStatusMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9LYWZrYUpvYlN0YXR1c01vbml0b3IuamF2YQ==) | | |
   | [...ce/extractor/extract/restapi/RestApiConnector.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9yZXN0YXBpL1Jlc3RBcGlDb25uZWN0b3IuamF2YQ==) | | |
   | [...in/recordaccess/RecordAccessorProviderFactory.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3JlY29yZGFjY2Vzcy9SZWNvcmRBY2Nlc3NvclByb3ZpZGVyRmFjdG9yeS5qYXZh) | | |
   | [...e/avro/AvroKeyRecursiveCombineFileInputFormat.java](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vbWFwcmVkdWNlL2F2cm8vQXZyb0tleVJlY3Vyc2l2ZUNvbWJpbmVGaWxlSW5wdXRGb3JtYXQuamF2YQ==) | | |
   | ... and [1667 more](https://codecov.io/gh/apache/gobblin/pull/3500/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/gobblin/pull/3500?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/gobblin/pull/3500?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [48b25a2...7e51d33](https://codecov.io/gh/apache/gobblin/pull/3500?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [gobblin] phet commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
phet commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r874382410


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/DefaultGobblinJobMetricReporter.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.base.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.metrics.ContextAwareGauge;
+import org.apache.gobblin.metrics.MetricContext;
+import org.apache.gobblin.metrics.ServiceMetricNames;
+import org.apache.gobblin.metrics.event.TimingEvent;
+import org.apache.gobblin.runtime.JobState;
+
+
+/**
+ * A metrics reporter that reports only workunitsCreationTimer - which is the current default behavior
+ *
+ */
+public class DefaultGobblinJobMetricReporter implements GobblinJobMetricReporter {
+
+  private Optional<MetricContext> metricContext;
+
+  public DefaultGobblinJobMetricReporter(Optional<MetricContext> metricContext) {
+     this.metricContext = metricContext;
+  }
+
+  public void reportWorkUnitCreationTimerMetrics(TimingEvent workUnitsCreationTimer, JobState jobState) {
+    if (!this.metricContext.isPresent()) {

Review Comment:
   don't we need to conditionally heed 'output job level metrics' (flag)?



##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/DefaultGobblinJobMetricReporter.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.base.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.metrics.ContextAwareGauge;
+import org.apache.gobblin.metrics.MetricContext;
+import org.apache.gobblin.metrics.ServiceMetricNames;
+import org.apache.gobblin.metrics.event.TimingEvent;
+import org.apache.gobblin.runtime.JobState;
+
+
+/**
+ * A metrics reporter that reports only workunitsCreationTimer - which is the current default behavior
+ *
+ */
+public class DefaultGobblinJobMetricReporter implements GobblinJobMetricReporter {

Review Comment:
   possibly clearer name: `LegacyGobblinJobMetricReporter`?
   
   ...at least I got the feeling you intend to transition to the `ServiceGobblinJobMetricReporter`, no?



##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/ServiceGobblinJobMetricReporter.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.base.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.metrics.ContextAwareGauge;
+import org.apache.gobblin.metrics.MetricContext;
+import org.apache.gobblin.metrics.ServiceMetricNames;
+import org.apache.gobblin.metrics.event.JobEvent;
+import org.apache.gobblin.metrics.event.TimingEvent;
+import org.apache.gobblin.runtime.JobState;
+
+
+/**
+ * A metrics reporter to report job-level metrics to Gobblin-as-a-Service
+ * Metrics should have the name FLOW_GROUP.FLOW_NAME.EDGE_ID.METRIC_NAME
+ * If edge ID does not exist due to a different flowgraph being used, use the jobName as default
+ */
+public class ServiceGobblinJobMetricReporter implements GobblinJobMetricReporter {
+  static String FLOW_EDGE_ID_KEY = "flow.edge.id";
+  private Optional<MetricContext> metricContext;
+
+  public ServiceGobblinJobMetricReporter(Optional<MetricContext> metricContext) {
+    this.metricContext = metricContext;
+  }
+
+  public void reportWorkUnitCreationTimerMetrics(TimingEvent workUnitsCreationTimer, JobState jobState) {
+    if (!this.metricContext.isPresent() || !jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+      return;
+    }
+    String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, jobState.getProp(ConfigurationKeys.FLOW_GROUP_KEY),
+        jobState.getProp(ConfigurationKeys.FLOW_NAME_KEY), jobState.getProp(FLOW_EDGE_ID_KEY, jobState.getJobName()), TimingEvent.LauncherTimings.WORK_UNITS_CREATION);
+    long workUnitsCreationTime = workUnitsCreationTimer.getDuration() / TimeUnit.SECONDS.toMillis(1);
+    ContextAwareGauge<Integer> workunitCreationGauge =
+        this.metricContext.get().newContextAwareGauge(workunitCreationGaugeName, () -> (int) workUnitsCreationTime);
+    this.metricContext.get().register(workunitCreationGaugeName, workunitCreationGauge);
+  }
+
+  public void reportWorkUnitCountMetrics(int workUnitCount, JobState jobState) {
+    if (!this.metricContext.isPresent() || !jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+      return;
+    }
+    String workunitCountGaugeName =  MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, jobState.getProp(ConfigurationKeys.FLOW_GROUP_KEY),
+        jobState.getProp(ConfigurationKeys.FLOW_NAME_KEY), jobState.getProp(FLOW_EDGE_ID_KEY, jobState.getJobName()), JobEvent.WORK_UNITS_CREATED);
+    ContextAwareGauge<Integer> workunitCountGauge = this.metricContext.get()
+        .newContextAwareGauge(workunitCountGaugeName, () -> Integer.valueOf(workUnitCount));

Review Comment:
   why the boxing here, but not above with the `workUnitsCreationTime`?



##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/metrics/GobblinJobMetricReporterTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.Metric;
+import java.io.FileReader;
+import java.io.IOException;
+import java.util.Map;
+import java.util.Properties;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.metastore.FsStateStore;
+import org.apache.gobblin.metastore.StateStore;
+import org.apache.gobblin.metastore.testing.ITestMetastoreDatabase;
+import org.apache.gobblin.metastore.testing.TestMetastoreDatabaseFactory;
+import org.apache.gobblin.runtime.JobContext;
+import org.apache.gobblin.runtime.JobLauncherTestHelper;
+import org.apache.gobblin.runtime.JobState;
+import org.apache.gobblin.runtime.local.LocalJobLauncher;
+import org.junit.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+
+/**
+ * Unit test for {@link LocalJobLauncher}.
+ *
+ * @author Yinan Li
+ */
+@Test(groups = { "gobblin.runtime.local" })
+public class GobblinJobMetricReporterTest {

Review Comment:
   javadoc out of date.
   
   BTW, what's the difference here with what I just read in `LocalJobLauncher`?  `testLaunchJob...` looked quite similar--did I miss a subtlety?



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r865432706


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,8 +674,23 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed
       if (this.jobContext.getJobMetricsOptional().isPresent()) {
+        if (jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+          String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,
+              TimingEvent.LauncherTimings.WORK_UNITS_CREATION, jobState.getJobName());
+          long workUnitsCreationTime = workUnitsCreationTimer.getDuration() / TimeUnit.SECONDS.toMillis(1);

Review Comment:
   Yeah there might be a better method of handling the metrics rather in finally, but I do think it is important to always emit metrics for each execution of a pipeline hence why trying to consolidate them all within a finally block. Maybe the solution here is to add yet another `try` in the `finally`? Reason being is that users should also have visibility when their workflow does not perform work, rather than not receiving information at all which would not activate alerting.
   
   



-- 
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

[GitHub] [gobblin] umustafi commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r866147253


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,8 +674,23 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed
       if (this.jobContext.getJobMetricsOptional().isPresent()) {
+        if (jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+          String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,

Review Comment:
   I thought you are moving workunit count emission to JobLauncher to prevent count from being emitted from each host. However, doesn't the gauge need to have the flow group/name still in the name? Otherwise how are these metrics tied to a particular flow upon emission? 



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r876423570


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,7 +680,7 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed

Review Comment:
   Ah good point, will revert this comment now 



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r875224740


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/DefaultGobblinJobMetricReporter.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.base.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.metrics.ContextAwareGauge;
+import org.apache.gobblin.metrics.MetricContext;
+import org.apache.gobblin.metrics.ServiceMetricNames;
+import org.apache.gobblin.metrics.event.TimingEvent;
+import org.apache.gobblin.runtime.JobState;
+
+
+/**
+ * A metrics reporter that reports only workunitsCreationTimer - which is the current default behavior
+ *
+ */
+public class DefaultGobblinJobMetricReporter implements GobblinJobMetricReporter {
+
+  private Optional<MetricContext> metricContext;
+
+  public DefaultGobblinJobMetricReporter(Optional<MetricContext> metricContext) {
+     this.metricContext = metricContext;
+  }
+
+  public void reportWorkUnitCreationTimerMetrics(TimingEvent workUnitsCreationTimer, JobState jobState) {
+    if (!this.metricContext.isPresent()) {

Review Comment:
   This reporter won't be used with GaaS job and only GaaS provides that configuration



-- 
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

[GitHub] [gobblin] ZihanLi58 commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r865392351


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,8 +674,23 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed
       if (this.jobContext.getJobMetricsOptional().isPresent()) {
+        if (jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+          String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,
+              TimingEvent.LauncherTimings.WORK_UNITS_CREATION, jobState.getJobName());
+          long workUnitsCreationTime = workUnitsCreationTimer.getDuration() / TimeUnit.SECONDS.toMillis(1);
+          ContextAwareGauge<Integer> workunitCreationGauge = this.runtimeMetricContext.get()
+              .newContextAwareGauge(workunitCreationGaugeName, () -> (int) workUnitsCreationTime);
+          this.runtimeMetricContext.get().register(workunitCreationGaugeName, workunitCreationGauge);
+
+          String workunitCountGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,

Review Comment:
   We change the guage name here, seems like one backward incompatible change and might require user communication. We also need to remove old rrd



-- 
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

[GitHub] [gobblin] phet commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
phet commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r875152527


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/DefaultGobblinJobMetricReporter.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.base.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.metrics.ContextAwareGauge;
+import org.apache.gobblin.metrics.MetricContext;
+import org.apache.gobblin.metrics.ServiceMetricNames;
+import org.apache.gobblin.metrics.event.TimingEvent;
+import org.apache.gobblin.runtime.JobState;
+
+
+/**
+ * A metrics reporter that reports only workunitsCreationTimer - which is the current default behavior
+ *
+ */
+public class DefaultGobblinJobMetricReporter implements GobblinJobMetricReporter {

Review Comment:
   cool.  much clearer for me now, so probably helpful to include in the javadoc



-- 
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

[GitHub] [gobblin] ZihanLi58 commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r876371194


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,7 +680,7 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed

Review Comment:
   what's this command for? seems outdated?



-- 
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

[GitHub] [gobblin] phet commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
phet commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r865400661


##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/KafkaJobStatusMonitor.java:
##########
@@ -191,11 +181,6 @@ protected void processMessage(DecodeableKafkaRecord<byte[],byte[]> message) {
       }
     }
 
-    if (gobblinTrackingEvent.getName().equals(JobEvent.WORK_UNITS_CREATED)) {
-      emitWorkUnitCountMetric(gobblinTrackingEvent);
-      return;
-    }

Review Comment:
   please explain what semantics we're changing by removing this... and why
   
   e.g. were we previously 'mirroring'/'repeating' the WU event publication on gaas, in addition to what the executor already published?



##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,8 +674,23 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed
       if (this.jobContext.getJobMetricsOptional().isPresent()) {
+        if (jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+          String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,
+              TimingEvent.LauncherTimings.WORK_UNITS_CREATION, jobState.getJobName());
+          long workUnitsCreationTime = workUnitsCreationTimer.getDuration() / TimeUnit.SECONDS.toMillis(1);

Review Comment:
   all that's here seems potentially excessive for a `finally` block.  in general, it's better reserved for managing resource lifecycle, rather than to perform a bunch of initialization/calculation that may itself throw an exception (when `finally` could already be unwinding from one).
   
   some specific Qs:
   a. is there a reason, say timing imprecision when calling `getDuration()`, not to keep within the `try`?
   b. does the event really need to be submitted even if there was an error/exception?



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r875228696


##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/metrics/GobblinJobMetricReporterTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.Metric;
+import java.io.FileReader;
+import java.io.IOException;
+import java.util.Map;
+import java.util.Properties;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.metastore.FsStateStore;
+import org.apache.gobblin.metastore.StateStore;
+import org.apache.gobblin.metastore.testing.ITestMetastoreDatabase;
+import org.apache.gobblin.metastore.testing.TestMetastoreDatabaseFactory;
+import org.apache.gobblin.runtime.JobContext;
+import org.apache.gobblin.runtime.JobLauncherTestHelper;
+import org.apache.gobblin.runtime.JobState;
+import org.apache.gobblin.runtime.local.LocalJobLauncher;
+import org.junit.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+
+/**
+ * Unit test for {@link LocalJobLauncher}.
+ *
+ * @author Yinan Li
+ */
+@Test(groups = { "gobblin.runtime.local" })
+public class GobblinJobMetricReporterTest {

Review Comment:
   Oh I'll update this thanks, I used the same testing interface but it's focused around testing the metric naming conventions rather than the job launcher logic itself. Not sure if it makes sense for me to make different tests for each metric reporter class, maybe I should do that instead?



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r865433580


##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/KafkaJobStatusMonitor.java:
##########
@@ -191,11 +181,6 @@ protected void processMessage(DecodeableKafkaRecord<byte[],byte[]> message) {
       }
     }
 
-    if (gobblinTrackingEvent.getName().equals(JobEvent.WORK_UNITS_CREATED)) {
-      emitWorkUnitCountMetric(gobblinTrackingEvent);
-      return;
-    }

Review Comment:
   Before the executor did not publish this metric, and I'm proposing that it does instead of the JobMonitor.
   Since the job monitor runs on every GaaS instance, for a single pipeline the metrics would be emitted for each of the GaaS hosts multiple times. By moving it to the executor, it only gets emitted once by a single host which makes the metric much more cleaner to track.



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r865430576


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,8 +674,23 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed
       if (this.jobContext.getJobMetricsOptional().isPresent()) {
+        if (jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+          String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,
+              TimingEvent.LauncherTimings.WORK_UNITS_CREATION, jobState.getJobName());
+          long workUnitsCreationTime = workUnitsCreationTimer.getDuration() / TimeUnit.SECONDS.toMillis(1);
+          ContextAwareGauge<Integer> workunitCreationGauge = this.runtimeMetricContext.get()
+              .newContextAwareGauge(workunitCreationGaugeName, () -> (int) workUnitsCreationTime);
+          this.runtimeMetricContext.get().register(workunitCreationGaugeName, workunitCreationGauge);
+
+          String workunitCountGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,

Review Comment:
   I think the old workunit count name wasn't being actively used according to SRE since it was being emitted by multiple hosts.



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r866190333


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java:
##########
@@ -685,8 +674,23 @@ public void apply(JobListener jobListener, JobContext jobContext)
         }
       }
     } finally {
-      // Stop metrics reporting
+      // Register metrics then stop metrics reporting, metrics will flush and emit when the metricsContext is closed
       if (this.jobContext.getJobMetricsOptional().isPresent()) {
+        if (jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+          String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_JOB_METRICS_PREFIX,

Review Comment:
   Yeah maybe we can explore this more in the doc, the jobName itself contains the flowName and flowGroup but also the edge name, which is important for the multihop usecase, where the # of workunits can differ.



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r874472761


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/DefaultGobblinJobMetricReporter.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.base.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.metrics.ContextAwareGauge;
+import org.apache.gobblin.metrics.MetricContext;
+import org.apache.gobblin.metrics.ServiceMetricNames;
+import org.apache.gobblin.metrics.event.TimingEvent;
+import org.apache.gobblin.runtime.JobState;
+
+
+/**
+ * A metrics reporter that reports only workunitsCreationTimer - which is the current default behavior
+ *
+ */
+public class DefaultGobblinJobMetricReporter implements GobblinJobMetricReporter {

Review Comment:
   This is used for any Gobblin Job **not** orchestrated by GaaS but still maintaining backwards compatibility, so hence the naming.



-- 
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

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3500: [GOBBLIN-1639] Prevent metrics reporting if configured, clean up workunit count metric

Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3500:
URL: https://github.com/apache/gobblin/pull/3500#discussion_r875227464


##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/metrics/ServiceGobblinJobMetricReporter.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.runtime.metrics;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.common.base.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.metrics.ContextAwareGauge;
+import org.apache.gobblin.metrics.MetricContext;
+import org.apache.gobblin.metrics.ServiceMetricNames;
+import org.apache.gobblin.metrics.event.JobEvent;
+import org.apache.gobblin.metrics.event.TimingEvent;
+import org.apache.gobblin.runtime.JobState;
+
+
+/**
+ * A metrics reporter to report job-level metrics to Gobblin-as-a-Service
+ * Metrics should have the name FLOW_GROUP.FLOW_NAME.EDGE_ID.METRIC_NAME
+ * If edge ID does not exist due to a different flowgraph being used, use the jobName as default
+ */
+public class ServiceGobblinJobMetricReporter implements GobblinJobMetricReporter {
+  static String FLOW_EDGE_ID_KEY = "flow.edge.id";
+  private Optional<MetricContext> metricContext;
+
+  public ServiceGobblinJobMetricReporter(Optional<MetricContext> metricContext) {
+    this.metricContext = metricContext;
+  }
+
+  public void reportWorkUnitCreationTimerMetrics(TimingEvent workUnitsCreationTimer, JobState jobState) {
+    if (!this.metricContext.isPresent() || !jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+      return;
+    }
+    String workunitCreationGaugeName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, jobState.getProp(ConfigurationKeys.FLOW_GROUP_KEY),
+        jobState.getProp(ConfigurationKeys.FLOW_NAME_KEY), jobState.getProp(FLOW_EDGE_ID_KEY, jobState.getJobName()), TimingEvent.LauncherTimings.WORK_UNITS_CREATION);
+    long workUnitsCreationTime = workUnitsCreationTimer.getDuration() / TimeUnit.SECONDS.toMillis(1);
+    ContextAwareGauge<Integer> workunitCreationGauge =
+        this.metricContext.get().newContextAwareGauge(workunitCreationGaugeName, () -> (int) workUnitsCreationTime);
+    this.metricContext.get().register(workunitCreationGaugeName, workunitCreationGauge);
+  }
+
+  public void reportWorkUnitCountMetrics(int workUnitCount, JobState jobState) {
+    if (!this.metricContext.isPresent() || !jobState.getPropAsBoolean(ConfigurationKeys.GOBBLIN_OUTPUT_JOB_LEVEL_METRICS, true)) {
+      return;
+    }
+    String workunitCountGaugeName =  MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, jobState.getProp(ConfigurationKeys.FLOW_GROUP_KEY),
+        jobState.getProp(ConfigurationKeys.FLOW_NAME_KEY), jobState.getProp(FLOW_EDGE_ID_KEY, jobState.getJobName()), JobEvent.WORK_UNITS_CREATED);
+    ContextAwareGauge<Integer> workunitCountGauge = this.metricContext.get()
+        .newContextAwareGauge(workunitCountGaugeName, () -> Integer.valueOf(workUnitCount));

Review Comment:
   workUnitsCreationTime is a long so it isn't supported in `Integer.valueOf`



-- 
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