Skip to content

[Feature][Metrics] Tag workflow related metrics with process definition code (workflow id) #13640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
EricGao888 merged 6 commits into from
Mar 9, 2023
Merged

[Feature][Metrics] Tag workflow related metrics with process definition code (workflow id) #13640

EricGao888 merged 6 commits into from
Mar 9, 2023

Conversation

@EricGao888
Copy link
Member

@EricGao888 EricGao888 commented Feb 27, 2023 •
edited
Loading

Purpose of the pull request

Brief change log

  • Already stated above

Verify this pull request

  • Verified by UT cases and manual tests.

@EricGao888 EricGao888 added feature new feature 3.2.0 for 3.2.0 version labels Feb 27, 2023
@EricGao888 EricGao888 added this to the 3.2.0 milestone Feb 27, 2023
@EricGao888 EricGao888 self-assigned this Feb 27, 2023
@EricGao888
Copy link
Member Author

EricGao888 commented Feb 27, 2023 •
edited
Loading

TODOs:

  • Remove related counters when deleting workflows.
  • Update related UT cases.
  • Update related docs.
  • Add related examples in grafana-demos.

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2023 •
edited
Loading

Codecov Report

Merging #13640 (07ac78d) into dev (9668c84) will decrease coverage by 0.03%.
The diff coverage is 28.07%.

❗ Current head 07ac78d differs from pull request most recent head 0968b99. Consider uploading reports for the commit 0968b99 to get more accurate results

@@             Coverage Diff              @@
##                dev   #13640      +/-   ##
============================================
- Coverage     39.08%   39.06%   -0.03%     
+ Complexity     4425     4423       -2     
============================================
  Files          1127     1130       +3     
  Lines         42096    42099       +3     
  Branches       4771     4775       +4     
============================================
- Hits          16452    16444       -8     
- Misses        23826    23841      +15     
+ Partials       1818     1814       -4
Impacted Files Coverage Δ
...che/dolphinscheduler/api/python/PythonGateway.java 17.24% <0.00%> (-0.23%) ⬇️
...heduler/api/service/impl/ResourcesServiceImpl.java 38.45% <ø> (-0.06%) ⬇️
...cheduler/common/constants/DataSourceConstants.java 0.00% <ø> (ø)
...server/master/event/WorkflowStartEventHandler.java 0.00% <0.00%> (ø)
...server/master/event/WorkflowStateEventHandler.java 0.00% <0.00%> (ø)
...master/event/WorkflowTimeoutStateEventHandler.java 0.00% <0.00%> (ø)
...r/server/master/processor/StateEventProcessor.java 0.00% <0.00%> (ø)
...er/server/master/processor/TaskEventProcessor.java 0.00% <0.00%> (ø)
...master/processor/TaskExecuteResponseProcessor.java 0.00% <0.00%> (ø)
...r/server/master/processor/TaskRecallProcessor.java 0.00% <0.00%> (ø)
... and 71 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@EricGao888 EricGao888 marked this pull request as ready for review March 7, 2023 06:26
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 7, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

23.3% 23.3% Coverage
0.0% 0.0% Duplication

ruanwenjun
ruanwenjun reviewed Mar 7, 2023
View reviewed changes
.../src/main/java/org/apache/dolphinscheduler/server/master/metrics/ProcessInstanceMetrics.java
public void incProcessInstanceByState(final String state) {
processInstanceCounters.get(state).increment();
public void incProcessInstanceByStateAndProcessDefinitionCode(final String state,
final String processDefinitionCode) {
Copy link
Member

@ruanwenjun ruanwenjun Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use workflowDefinitionName rather than code here?

Copy link
Member Author

@EricGao888 EricGao888 Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

DROP TABLE IF EXISTS `t_ds_process_definition`;
CREATE TABLE `t_ds_process_definition` (
  `id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'self-increasing id',
  `code` bigint(20) NOT NULL COMMENT 'encoding',
  `name` varchar(255) DEFAULT NULL COMMENT 'process definition name',
  `version` int(11) DEFAULT '0' COMMENT 'process definition version',
  `description` text COMMENT 'description',
  `project_code` bigint(20) NOT NULL COMMENT 'project code',
  `release_state` tinyint(4) DEFAULT NULL COMMENT 'process definition release state：0:offline,1:online',
  `user_id` int(11) DEFAULT NULL COMMENT 'process definition creator id',
  `global_params` text COMMENT 'global parameters',
  `flag` tinyint(4) DEFAULT NULL COMMENT '0 not available, 1 available',
  `locations` text COMMENT 'Node location information',
  `warning_group_id` int(11) DEFAULT NULL COMMENT 'alert group id',
  `timeout` int(11) DEFAULT '0' COMMENT 'time out, unit: minute',
  `tenant_id` int(11) NOT NULL DEFAULT '-1' COMMENT 'tenant id',
  `execution_type` tinyint(4) DEFAULT '0' COMMENT 'execution_type 0:parallel,1:serial wait,2:serial discard,3:serial priority',
  `create_time` datetime NOT NULL COMMENT 'create time',
  `update_time` datetime NOT NULL COMMENT 'update time',
  PRIMARY KEY (`id`,`code`),
  UNIQUE KEY `process_unique` (`name`,`project_code`) USING BTREE
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8 COLLATE = utf8_bin;

Looks like there could be duplicate workflow definition names in two different projects. Changing tag to it might lead to miscounting.

Copy link
Member

@ruanwenjun ruanwenjun Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙆‍♂️

@EricGao888 EricGao888 requested a review from ruanwenjun March 8, 2023 02:35
ruanwenjun
ruanwenjun approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@EricGao888
Copy link
Member Author

EricGao888 commented Mar 8, 2023

@kezhenxu94 Would u like to double check when available? Thx

@EricGao888 EricGao888 merged commit 394805b into apache:dev Mar 9, 2023
@EricGao888 EricGao888 mentioned this pull request Mar 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ruanwenjun ruanwenjun ruanwenjun approved these changes

@zhongjiajie zhongjiajie Awaiting requested review from zhongjiajie

@Tianqi-Dotes Tianqi-Dotes Awaiting requested review from Tianqi-Dotes

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

@kezhenxu94 kezhenxu94 Awaiting requested review from kezhenxu94

Assignees

@EricGao888 EricGao888

Labels

3.2.0 for 3.2.0 version backend document feature new feature

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

[Feature][Metrics] Add metrics for workflow runtime

3 participants

@EricGao888 @codecov-commenter @ruanwenjun