You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2022/12/13 14:47:37 UTC
[GitHub] [dolphinscheduler] acongfly opened a new pull request, #13183: [Feature] TaskMetrics add taskName metric #13111
acongfly opened a new pull request, #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183
<!--Thanks very much for contributing to Apache DolphinScheduler. Please review https://dolphinscheduler.apache.org/en-us/community/development/pull-request.html before opening a pull request.-->
## Purpose of the pull request
I see that the following issues are somewhat similar to our recent requirements. Try to implement it. This is the relevant code. Please help me to check whether it can meet the requirements. My local test here meets our needs.
[Feature][Metrics] task fail metrics #13111
<!--(For example: This pull request adds checkstyle plugin).-->
## Brief change log
<!--*(for example:)*
- *Add maven-checkstyle-plugin to root pom.xml*
-->
## Verify this pull request
<!--*(Please pick either of the following options)*-->
This pull request is code cleanup without any test coverage.
*(or)*
This pull request is already covered by existing tests, such as *(please describe tests)*.
(or)
This change added tests and can be verified as follows:
<!--*(example:)*
- *Added dolphinscheduler-dao tests for end-to-end.*
- *Added CronUtilsTest to verify the change.*
- *Manually verified the change by testing locally.* -->
(or)
If your pull request contain incompatible change, you should also add it to `docs/docs/en/guide/upgrede/incompatible.md`
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] EricGao888 commented on pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by GitBox <gi...@apache.org>.
EricGao888 commented on PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#issuecomment-1351064935
Hi @acongfly ,
Several months ago we've discussed about using task instance id / task instance name as a dimension of tags. There might be potential memory risk when task instances accumulated, see: https://github.com/apache/dolphinscheduler/issues/10525#issuecomment-1169954936
Compared to `task instance id`, `task instance name` is a better choice as there are much less distinct task names than task instance ids. BTW, you need to make sure task instance names are distinct, otherwise you might get incorrect data.
However, if you want to contribute this feature, I suggest u add filters / switch for generating these kind of `high cardinality` dimensions. For users scheduling tons of different tasks, this feature might cause side-effect.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] acongfly commented on pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by GitBox <gi...@apache.org>.
acongfly commented on PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#issuecomment-1359234646
A switch was added to re-implement this function. I'd like to get feedback If anything is not correct @EricGao888 @ruanwenjun @SbloodyS
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] github-actions[bot] commented on pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#issuecomment-1659383957
This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] EricGao888 commented on a diff in pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#discussion_r1139679300
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/metrics/TaskMetrics.java:
##########
@@ -83,11 +84,15 @@ public void incTaskDispatch() {
taskDispatchCounter.increment();
}
- public void incTaskInstanceByState(final String state) {
- if (taskInstanceCounters.get(state) == null) {
+ public void incTaskInstanceByState(final String state, String taskInstanceName) {
+ Counter.Builder builder = taskInstanceCountersBuild.get(state);
+ if (builder == null) {
return;
}
- taskInstanceCounters.get(state).increment();
+ if (MasterConfigStatic.masterConfigStatic.isSupportTaskNameTagMetric()) {
Review Comment:
https://github.com/apache/dolphinscheduler/blob/1ac76e1cbbf25156f3b40dc5035ef7b15dfc9bdf/dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_mysql.sql#L468-L503
`task name` is not identical, aggregating metrics by `task name` may lead to miscounting. `id` is identical for task definition, however, I haven't found a way for users to get `task definition id` easily from web ui.
##########
dolphinscheduler-standalone-server/src/main/resources/application.yaml:
##########
@@ -149,6 +149,8 @@ master:
# kill yarn/k8s application when failover taskInstance, default true
kill-application-when-task-failover: true
worker-group-refresh-interval: 10s
+ # This attribute is mainly supported by task metric, allowing task instance name to be used as a tag.
+ support-task-name-tag-metric: true
Review Comment:
Since the number of task keeps increasing, we need clean-up mechanism for metrics tagged with task id. Otherwise, users may face memory issue. You may refer to what we did in this PR https://github.com/apache/dolphinscheduler/pull/13640
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] acongfly commented on pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by GitBox <gi...@apache.org>.
acongfly commented on PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#issuecomment-1351363835
> Hi @acongfly ,
>
> Several months ago we've discussed about using task instance id / task instance name as a dimension of tags. There might be potential memory risk when task instances accumulated, see: [#10525 (comment)](https://github.com/apache/dolphinscheduler/issues/10525#issuecomment-1169954936)
>
> Compared to `task instance id`, `task instance name` is a better choice as there are much less distinct task names than task instance ids. BTW, you need to make sure task instance names are distinct, otherwise you might get incorrect data.
>
> However, if you want to contribute this feature, I suggest u add filters / switches for generating these kinds of `high cardinality` dimensions. For users scheduling tons of different tasks, this feature might cause side-effect.
@EricGao888
Thank you for your suggestion. I will take it into consideration and try to redesign the feature. If I have any questions, I will leave a message here. Thank you again.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] github-actions[bot] commented on pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#issuecomment-1646700618
This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] EricGao888 commented on a diff in pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#discussion_r1139679300
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/metrics/TaskMetrics.java:
##########
@@ -83,11 +84,15 @@ public void incTaskDispatch() {
taskDispatchCounter.increment();
}
- public void incTaskInstanceByState(final String state) {
- if (taskInstanceCounters.get(state) == null) {
+ public void incTaskInstanceByState(final String state, String taskInstanceName) {
+ Counter.Builder builder = taskInstanceCountersBuild.get(state);
+ if (builder == null) {
return;
}
- taskInstanceCounters.get(state).increment();
+ if (MasterConfigStatic.masterConfigStatic.isSupportTaskNameTagMetric()) {
Review Comment:
https://github.com/apache/dolphinscheduler/blob/1ac76e1cbbf25156f3b40dc5035ef7b15dfc9bdf/dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_mysql.sql#L468-L503
`task name` is not identical, aggregating metrics by `task name` may lead to miscounting. `task definition id` is identical for task definition, however, I haven't found a way for users to get `task definition id` easily from web ui.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] github-actions[bot] closed pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #13183: [Feature] Task monitoring supports task names as tags.
URL: https://github.com/apache/dolphinscheduler/pull/13183
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] acongfly commented on a diff in pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by GitBox <gi...@apache.org>.
acongfly commented on code in PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#discussion_r1048471000
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/metrics/TaskMetrics.java:
##########
@@ -83,11 +85,31 @@ public void incTaskDispatch() {
taskDispatchCounter.increment();
}
- public void incTaskInstanceByState(final String state) {
- if (taskInstanceCounters.get(state) == null) {
+ public void incTaskInstanceByState(final String state, Integer taskInstanceId) {
Review Comment:
Thank you for your suggestion. I will redesign this implementation.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a diff in pull request #13183: [Feature] Task monitoring supports task names as tags.
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #13183:
URL: https://github.com/apache/dolphinscheduler/pull/13183#discussion_r1047946161
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/metrics/TaskMetrics.java:
##########
@@ -83,11 +85,31 @@ public void incTaskDispatch() {
taskDispatchCounter.increment();
}
- public void incTaskInstanceByState(final String state) {
- if (taskInstanceCounters.get(state) == null) {
+ public void incTaskInstanceByState(final String state, Integer taskInstanceId) {
Review Comment:
```suggestion
public void incTaskInstanceByState(final String state, String taskInstanceName) {
```
##########
dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/metrics/TaskMetrics.java:
##########
@@ -83,11 +85,31 @@ public void incTaskDispatch() {
taskDispatchCounter.increment();
}
- public void incTaskInstanceByState(final String state) {
- if (taskInstanceCounters.get(state) == null) {
+ public void incTaskInstanceByState(final String state, Integer taskInstanceId) {
Review Comment:
Please don't query the database again.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org