disttask: make task executor `onError` print error's stack. #56618

LindaSummer · 2024-10-12T10:03:51Z

What problem does this PR solve?

Issue Number: close #56014

Problem Summary:

What changed and how does it work?

Error log in onError function's stack is always in the same function.

But the real stack should be the one invoking onError.

Check List

Tests

Unit test

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2024-10-12T10:04:03Z

Hi @LindaSummer. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiprow · 2024-10-12T10:04:15Z

Hi @LindaSummer. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

LindaSummer · 2024-10-13T01:51:58Z

Hi @lance6716 ,

Please take a look. 😊

Thanks very much.

Best Regards,
Edward

codecov · 2024-10-13T08:45:27Z

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 56.8286%. Comparing base (e13bfeb) to head (5420294).
Report is 13 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56618         +/-   ##
=================================================
- Coverage   73.3262%   56.8286%   -16.4977%     
=================================================
  Files          1630       1759        +129     
  Lines        450312     635618     +185306     
=================================================
+ Hits         330197     361213      +31016     
- Misses        99833     250289     +150456     
- Partials      20282      24116       +3834

Flag	Coverage Δ
integration	`37.0566% <20.0000%> (?)`
unit	`73.0720% <66.6666%> (+0.6119%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`53.0248% <ø> (+7.4360%)`	⬆️

LindaSummer · 2024-10-13T09:36:17Z

/ok-to-test

And please post the test result. Or if you can, add an unit test for it. For example, check the output of logger and verify it contents the function that throws the error

Thanks very much for your suggestions!

I will try to add a test case for it. 😊

Best Regards,
Edward

LindaSummer · 2024-10-14T15:08:21Z

Hi @lance6716 ,

I have corrected my mistake and add unit tests. 😊

Please take a look.

Best Regards,
Edward

lance6716

Please run make bazel_prepare to update .bazel files, or just apply the following git diff (run git apply - and paste following contents):

diff --git a/pkg/disttask/framework/taskexecutor/BUILD.bazel b/pkg/disttask/framework/taskexecutor/BUILD.bazel
index e65cc685e0..79a2483481 100644
--- a/pkg/disttask/framework/taskexecutor/BUILD.bazel
+++ b/pkg/disttask/framework/taskexecutor/BUILD.bazel
@@ -51,7 +51,7 @@ go_test(
     ],
     embed = [":taskexecutor"],
     flaky = True,
-    shard_count = 16,
+    shard_count = 17,
     deps = [
         "//pkg/disttask/framework/mock",
         "//pkg/disttask/framework/mock/execute",
@@ -74,5 +74,7 @@ go_test(
         "@org_golang_google_grpc//status",
         "@org_uber_go_goleak//:goleak",
         "@org_uber_go_mock//gomock",
+        "@org_uber_go_zap//:zap",
+        "@org_uber_go_zap//zaptest/observer",
     ],
 )

Or I can help you push that change if you have some problems.

pkg/disttask/framework/taskexecutor/task_executor_test.go

LindaSummer · 2024-10-15T02:21:14Z

Thanks very much for your warm suggestion and help!

I will update this PR later today.😄

Best Regards,
Edward

LindaSummer · 2024-10-15T13:29:25Z

Hi @lance6716 ,

Thanks very much for your bazel patch!😊

I have updated related code and added stacktrace explicitly for all onError invokers.

Best Regards,
Edward

LindaSummer · 2024-10-16T13:29:44Z

Hi @lance6716 ，

Thanks for your patience and warm help! 😊

I have added an error stack field for stack info inside error and skipped it for error without stack info.

Best Regards,
Edward

LindaSummer · 2024-10-16T13:52:38Z

/retest

lance6716

rest lgtm

pkg/disttask/framework/taskexecutor/task_executor_test.go

pkg/disttask/framework/taskexecutor/task_executor.go

LindaSummer · 2024-10-17T04:47:58Z

rest lgtm

Thanks very much for your patience and help! 😊

I'll update it later today.

Best Regards,
Edward

LindaSummer · 2024-10-17T14:45:35Z

/retest

LindaSummer · 2024-10-18T13:23:36Z

Hi @GMHDBJD,

Sorry to bother you. 😊

Could help take a look?

Best Regards,
Edward

LindaSummer · 2024-10-18T13:55:11Z

/retest

LindaSummer · 2024-10-19T07:28:10Z

/retest

LindaSummer · 2024-10-19T08:00:11Z

/retest

Benjamin2037

LGTM

ti-chi-bot · 2024-10-22T02:37:37Z

[LGTM Timeline notifier]

Timeline:

2024-10-17 13:49:07.952007338 +0000 UTC m=+538145.100917175: ☑️ agreed by lance6716.
2024-10-22 02:37:36.911991612 +0000 UTC m=+318657.608782236: ☑️ agreed by Benjamin2037.

LindaSummer · 2024-10-22T02:55:18Z

/assign

ti-chi-bot · 2024-10-22T03:12:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, CbcWestwolf, lance6716

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/disttask/OWNERS~~ [Benjamin2037,lance6716]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

D3Hunter · 2024-10-22T03:23:15Z

pkg/disttask/framework/taskexecutor/task_executor.go

-	err = errors.Trace(err)
-	e.logger.Error("onError", zap.Error(err), zap.Stack("stack"))
+
+	if errors.HasStack(err) {


why check it?

we don't need to print the plain error stack. There's already zap.Error(err)

D3Hunter · 2024-10-22T03:24:05Z

/hold

D3Hunter · 2024-10-22T05:09:38Z

/unhold

make task executor onerror log skip one more level of stack.

d0c6277

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Oct 12, 2024

This comment was marked as resolved.

Sign in to view

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Oct 13, 2024

fix log stack skip and add unit test.

cfbc539

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 14, 2024

lance6716 reviewed Oct 15, 2024

View reviewed changes

pkg/disttask/framework/taskexecutor/task_executor_test.go Outdated Show resolved Hide resolved

fix bazel build and add errors.Trace before onError.

63b6f41

LindaSummer and others added 2 commits October 15, 2024 21:29

Merge branch 'master' into chore/make_on_error_stack_skip_self

3441ad0

add "error stack" for original stack info inside error.

8b76a80

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2024

Merge branch 'master' into chore/make_on_error_stack_skip_self

1e75b3b

lance6716 reviewed Oct 17, 2024

View reviewed changes

remove useless stack checking.

4090054

ti-chi-bot bot requested a review from GMHDBJD October 17, 2024 13:13

remove erros.Trace for fixed errors.

e243162

lance6716 approved these changes Oct 17, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 17, 2024

Merge branch 'master' into chore/make_on_error_stack_skip_self

3f3fc02

LindaSummer changed the title ~~disttask: make task executor onerror log skip one more level of stack.~~ disttask: make task executor onError print error's stack. Oct 18, 2024

Merge branch 'master' into chore/make_on_error_stack_skip_self

98fd3ac

LindaSummer added 2 commits October 18, 2024 22:01

Merge branch 'master' into chore/make_on_error_stack_skip_self

bc5411e

Merge branch 'master' into chore/make_on_error_stack_skip_self

c807e8c

Merge branch 'master' into chore/make_on_error_stack_skip_self

5420294

Benjamin2037 approved these changes Oct 22, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 22, 2024

ti-chi-bot bot assigned LindaSummer Oct 22, 2024

CbcWestwolf approved these changes Oct 22, 2024

View reviewed changes

D3Hunter reviewed Oct 22, 2024

View reviewed changes

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 22, 2024

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 22, 2024

ti-chi-bot bot merged commit a00ba68 into pingcap:master Oct 22, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disttask: make task executor `onError` print error's stack. #56618

disttask: make task executor `onError` print error's stack. #56618

LindaSummer commented Oct 12, 2024

ti-chi-bot bot commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

LindaSummer commented Oct 13, 2024

This comment was marked as resolved.

codecov bot commented Oct 13, 2024 •

edited

Loading

LindaSummer commented Oct 13, 2024

LindaSummer commented Oct 14, 2024

lance6716 left a comment •

edited

Loading

LindaSummer commented Oct 15, 2024

LindaSummer commented Oct 15, 2024

LindaSummer commented Oct 16, 2024

LindaSummer commented Oct 16, 2024

lance6716 left a comment

LindaSummer commented Oct 17, 2024

LindaSummer commented Oct 17, 2024

LindaSummer commented Oct 18, 2024

LindaSummer commented Oct 18, 2024

LindaSummer commented Oct 19, 2024

LindaSummer commented Oct 19, 2024

Benjamin2037 left a comment

ti-chi-bot bot commented Oct 22, 2024

LindaSummer commented Oct 22, 2024

ti-chi-bot bot commented Oct 22, 2024

D3Hunter Oct 22, 2024

lance6716 Oct 22, 2024

D3Hunter commented Oct 22, 2024

D3Hunter commented Oct 22, 2024

disttask: make task executor onError print error's stack. #56618

disttask: make task executor onError print error's stack. #56618

Conversation

LindaSummer commented Oct 12, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

LindaSummer commented Oct 13, 2024

This comment was marked as resolved.

codecov bot commented Oct 13, 2024 • edited Loading

Codecov Report

LindaSummer commented Oct 13, 2024

LindaSummer commented Oct 14, 2024

lance6716 left a comment • edited Loading

Choose a reason for hiding this comment

LindaSummer commented Oct 15, 2024

LindaSummer commented Oct 15, 2024

LindaSummer commented Oct 16, 2024

LindaSummer commented Oct 16, 2024

lance6716 left a comment

Choose a reason for hiding this comment

LindaSummer commented Oct 17, 2024

LindaSummer commented Oct 17, 2024

LindaSummer commented Oct 18, 2024

LindaSummer commented Oct 18, 2024

LindaSummer commented Oct 19, 2024

LindaSummer commented Oct 19, 2024

Benjamin2037 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 22, 2024

[LGTM Timeline notifier]

LindaSummer commented Oct 22, 2024

ti-chi-bot bot commented Oct 22, 2024

D3Hunter Oct 22, 2024

Choose a reason for hiding this comment

lance6716 Oct 22, 2024

Choose a reason for hiding this comment

D3Hunter commented Oct 22, 2024

D3Hunter commented Oct 22, 2024

disttask: make task executor `onError` print error's stack. #56618

disttask: make task executor `onError` print error's stack. #56618

codecov bot commented Oct 13, 2024 •

edited

Loading

lance6716 left a comment •

edited

Loading