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/05 16:15:19 UTC
[GitHub] [gobblin] Will-Lo opened a new pull request, #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Will-Lo opened a new pull request, #3502:
URL: https://github.com/apache/gobblin/pull/3502
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
- [ ] 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-1641
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
Currently the dag manager does not have metrics around how many flows are being exceeded per executor, so we should track this.
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [ ] 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] Will-Lo commented on a diff in pull request #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#discussion_r866274607
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -457,7 +460,8 @@ public static class DagManagerThread implements Runnable {
DagManagerThread(JobStatusRetriever jobStatusRetriever, DagStateStore dagStateStore, DagStateStore failedDagStateStore,
BlockingQueue<Dag<JobExecutionPlan>> queue, BlockingQueue<String> cancelQueue, BlockingQueue<String> resumeQueue,
boolean instrumentationEnabled, Set<String> failedDagIds, ContextAwareMeter allSuccessfulMeter,
- ContextAwareMeter allFailedMeter, Long defaultJobStartSla, UserQuotaManager quotaManager, int dagMangerThreadId) {
+ ContextAwareMeter allFailedMeter, Map<String, ContextAwareMeter> startSlaExceededMeters,
Review Comment:
Yes I was thinking we can create some class later with additional metrics reporting and abstract all this out of the DagManager, a good chunk of the DagManager code is now metrics related
--
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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#discussion_r866274863
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -1098,6 +1105,19 @@ private ContextAwareMeter getGroupMeterForDag(String dagId, String meterName, Ma
group -> metricContext.contextAwareMeter(MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, group, meterName)));
}
+ /**
+ * Used to track metrics for different specExecutors to detect issues with the specExecutor itself
+ * @param dagNode
+ * @param meterName
+ * @param meterMap
+ * @return
+ */
+ private ContextAwareMeter getExecutorMeterForDag(DagNode<JobExecutionPlan> dagNode, String meterName, Map<String, ContextAwareMeter> meterMap) {
+ String executorName = dagNode.getValue().getSpecExecutor().getUri().toString();
Review Comment:
I can add that, that's a good point :)
--
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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
ZihanLi58 merged PR #3502:
URL: https://github.com/apache/gobblin/pull/3502
--
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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#discussion_r866274218
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -357,6 +357,8 @@ public synchronized void setActive(boolean active) {
ContextAwareMeter allSuccessfulMeter = null;
ContextAwareMeter allFailedMeter = null;
+ // TODO: create a map of RatioGauges for success/failed per executor, will require preprocessing of available executors
+ Map<String, ContextAwareMeter> startSlaExceededMeters = Maps.newConcurrentMap();
Review Comment:
a. I was thinking of a more robust metrics accounting that could do RatioGauges, which is an abstract class https://www.javadoc.io/static/io.dropwizard.metrics/metrics-core/3.1.2/com/codahale/metrics/RatioGauge.html that takes in 2 meters. I was thinking that if we were to also report ratios, then they need a reference to both the corresponding meters as well? Haven't thought it out all the way.
b. I was thinking with ratios we'd have percentage of successful/failed against all jobs, maybe percentage of jobs that are sla killed but there needs to be some flags needed since it gets lost after the jobstatusmonitor.
--
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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#discussion_r866274218
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -357,6 +357,8 @@ public synchronized void setActive(boolean active) {
ContextAwareMeter allSuccessfulMeter = null;
ContextAwareMeter allFailedMeter = null;
+ // TODO: create a map of RatioGauges for success/failed per executor, will require preprocessing of available executors
+ Map<String, ContextAwareMeter> startSlaExceededMeters = Maps.newConcurrentMap();
Review Comment:
a. I was thinking of a more robust metrics accounting that could do RatioGauges, which is an abstract class https://www.javadoc.io/static/io.dropwizard.metrics/metrics-core/3.1.2/com/codahale/metrics/RatioGauge.html that takes in 2 meters. If we were to also report ratios, then they need a reference to both the corresponding meters as well? Haven't thought it out all the way.
b. I was thinking with ratios we'd have percentage of successful/failed against all jobs, maybe percentage of jobs that are sla killed but there needs to be some flags needed since it gets lost after the jobstatusmonitor.
--
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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#discussion_r866274218
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -357,6 +357,8 @@ public synchronized void setActive(boolean active) {
ContextAwareMeter allSuccessfulMeter = null;
ContextAwareMeter allFailedMeter = null;
+ // TODO: create a map of RatioGauges for success/failed per executor, will require preprocessing of available executors
+ Map<String, ContextAwareMeter> startSlaExceededMeters = Maps.newConcurrentMap();
Review Comment:
a. I was thinking of a more robust metrics accounting that could do RatioGauges, which is an abstract class https://www.javadoc.io/static/io.dropwizard.metrics/metrics-core/3.1.2/com/codahale/metrics/RatioGauge.html that takes in 2 meters. I was thinking that if we were to also report ratios, then they need a reference to both the corresponding meters as well? Haven't thought it out all the way.
b. I was thinking with ratios we'd have percentage of successful/failed jobs
--
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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#issuecomment-1118781963
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3502?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 [#3502](https://codecov.io/gh/apache/gobblin/pull/3502?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3cc0c10) into [master](https://codecov.io/gh/apache/gobblin/commit/3a497969a8888fe730151fb5bfbcab281b15eeb2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3a49796) will **increase** coverage by `1.79%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #3502 +/- ##
============================================
+ Coverage 46.68% 48.48% +1.79%
+ Complexity 10405 7735 -2670
============================================
Files 2078 1443 -635
Lines 81288 57117 -24171
Branches 9078 6587 -2491
============================================
- Hits 37947 27691 -10256
+ Misses 39843 26845 -12998
+ Partials 3498 2581 -917
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...org/apache/gobblin/metrics/ServiceMetricNames.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL21ldHJpY3MvU2VydmljZU1ldHJpY05hbWVzLmphdmE=) | `0.00% <ø> (ø)` | |
| [.../java/org/apache/gobblin/cluster/SleepingTask.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2xlZXBpbmdUYXNrLmphdmE=) | `39.39% <0.00%> (-6.07%)` | :arrow_down: |
| [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/gobblin/pull/3502/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: |
| [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `29.54% <0.00%> (-0.33%)` | :arrow_down: |
| [...anagement/copy/replication/ConfigBasedDataset.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvcmVwbGljYXRpb24vQ29uZmlnQmFzZWREYXRhc2V0LmphdmE=) | `68.87% <0.00%> (ø)` | |
| [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/gobblin/pull/3502/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==) | | |
| [...pache/gobblin/source/jdbc/PostgresqlExtractor.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9qZGJjL1Bvc3RncmVzcWxFeHRyYWN0b3IuamF2YQ==) | | |
| [...bblin/converter/StringFieldEncryptorConverter.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tY3J5cHRvLXByb3ZpZGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9TdHJpbmdGaWVsZEVuY3J5cHRvckNvbnZlcnRlci5qYXZh) | | |
| [...lin/service/FlowExecutionResourceLocalHandler.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | | |
| [...bblin/util/limiter/stressTest/StressTestUtils.java](https://codecov.io/gh/apache/gobblin/pull/3502/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-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2UvZ29iYmxpbi10aHJvdHRsaW5nLXNlcnZpY2UtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvbGltaXRlci9zdHJlc3NUZXN0L1N0cmVzc1Rlc3RVdGlscy5qYXZh) | | |
| ... and [633 more](https://codecov.io/gh/apache/gobblin/pull/3502/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/3502?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/3502?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 [3a49796...3cc0c10](https://codecov.io/gh/apache/gobblin/pull/3502?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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
phet commented on code in PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#discussion_r866250207
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -457,7 +460,8 @@ public static class DagManagerThread implements Runnable {
DagManagerThread(JobStatusRetriever jobStatusRetriever, DagStateStore dagStateStore, DagStateStore failedDagStateStore,
BlockingQueue<Dag<JobExecutionPlan>> queue, BlockingQueue<String> cancelQueue, BlockingQueue<String> resumeQueue,
boolean instrumentationEnabled, Set<String> failedDagIds, ContextAwareMeter allSuccessfulMeter,
- ContextAwareMeter allFailedMeter, Long defaultJobStartSla, UserQuotaManager quotaManager, int dagMangerThreadId) {
+ ContextAwareMeter allFailedMeter, Map<String, ContextAwareMeter> startSlaExceededMeters,
Review Comment:
not mandatory now, but seeing there are already three `CAMeter`-related params makes me wonder whether a higher-level of abstraction might be in order (to collect them together).
perhaps wait and see?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -1098,6 +1105,19 @@ private ContextAwareMeter getGroupMeterForDag(String dagId, String meterName, Ma
group -> metricContext.contextAwareMeter(MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, group, meterName)));
}
+ /**
+ * Used to track metrics for different specExecutors to detect issues with the specExecutor itself
+ * @param dagNode
+ * @param meterName
+ * @param meterMap
+ * @return
+ */
+ private ContextAwareMeter getExecutorMeterForDag(DagNode<JobExecutionPlan> dagNode, String meterName, Map<String, ContextAwareMeter> meterMap) {
+ String executorName = dagNode.getValue().getSpecExecutor().getUri().toString();
+ return meterMap.computeIfAbsent(executorName,
+ executorUri -> metricContext.contextAwareMeter(MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, executorUri, meterName)));
Review Comment:
to turn this on its head, the map you're carrying around and passing into here is basically just function memoization for this `computeIfAbsent` call.
up to you, but I might raise the level of abstraction by presenting this as a cache of `ContextAwareMeter`s that delegates read-through (initialization) to the `MetricRegistry`.
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -357,6 +357,8 @@ public synchronized void setActive(boolean active) {
ContextAwareMeter allSuccessfulMeter = null;
ContextAwareMeter allFailedMeter = null;
+ // TODO: create a map of RatioGauges for success/failed per executor, will require preprocessing of available executors
+ Map<String, ContextAwareMeter> startSlaExceededMeters = Maps.newConcurrentMap();
Review Comment:
not clear what the `String` key reflects... hence where the todo fits against what's implemented now
my naming convention (clearly not what's used already in the project) is to name maps with a `byX` suffix, so e.g. `startSlaExceededMetersByExecutorId`
UPDATE: now, after reading the entire PR, I understand the TODO as the intent to expand into other per-executor metrics. the way you phrased it at the implementation level initially distracted me. so then, two Qs:
a. why is the pre-processing required, rather than the JIT initialization/vivication you do now (e.g. the `computeIfAbsent`)?
b. any other per-executor stats in mind?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -1098,6 +1105,19 @@ private ContextAwareMeter getGroupMeterForDag(String dagId, String meterName, Ma
group -> metricContext.contextAwareMeter(MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, group, meterName)));
}
+ /**
+ * Used to track metrics for different specExecutors to detect issues with the specExecutor itself
+ * @param dagNode
+ * @param meterName
+ * @param meterMap
+ * @return
+ */
+ private ContextAwareMeter getExecutorMeterForDag(DagNode<JobExecutionPlan> dagNode, String meterName, Map<String, ContextAwareMeter> meterMap) {
+ String executorName = dagNode.getValue().getSpecExecutor().getUri().toString();
Review Comment:
if we lack a `DagManagerUtils.getExecutorName`, should we add?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagManagerTest.java:
##########
@@ -736,6 +736,9 @@ public void testJobStartSLAKilledDag() throws URISyntaxException, IOException {
Assert.assertEquals(this.dagToJobs.size(), 1);
Assert.assertTrue(this.dags.containsKey(dagId1));
+ String slakilledMeterName = MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, "job0", ServiceMetricNames.START_SLA_EXCEEDED_FLOWS_METER);
+ Assert.assertEquals(metricContext.getParent().get().getMeters().get(slakilledMeterName).getCount(), 1);
Review Comment:
would it be worth verifying that meters are per-executor, not a single aggregation?
--
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 #3502: [GOBBLIN-1641] Add meter for sla exceeded flows
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3502:
URL: https://github.com/apache/gobblin/pull/3502#discussion_r866283702
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -1098,6 +1105,19 @@ private ContextAwareMeter getGroupMeterForDag(String dagId, String meterName, Ma
group -> metricContext.contextAwareMeter(MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, group, meterName)));
}
+ /**
+ * Used to track metrics for different specExecutors to detect issues with the specExecutor itself
+ * @param dagNode
+ * @param meterName
+ * @param meterMap
+ * @return
+ */
+ private ContextAwareMeter getExecutorMeterForDag(DagNode<JobExecutionPlan> dagNode, String meterName, Map<String, ContextAwareMeter> meterMap) {
+ String executorName = dagNode.getValue().getSpecExecutor().getUri().toString();
+ return meterMap.computeIfAbsent(executorName,
+ executorUri -> metricContext.contextAwareMeter(MetricRegistry.name(ServiceMetricNames.GOBBLIN_SERVICE_PREFIX, executorUri, meterName)));
Review Comment:
Ah yeah that's a good point it is essentially a cache 😓 . I think we can leave it as is for now but when we refactor the metrics out of the DagManager this should have a different interface along with the other metrics for some consolidation/consistency.
--
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