ddl: use package-level logger which preset "category" field #52885

lance6716 · 2024-04-25T10:07:45Z

What problem does this PR solve?

Issue Number: close #52883

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

$ tiup playground nightly --tiflash 0 --without-monitor --db.binpath ~/Projects/tidb/bin/tidb-server

check log

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

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

None

Signed-off-by: lance6716 <lance6716@gmail.com>

tiprow · 2024-04-25T10:08:04Z

Hi @lance6716. 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/test-infra repository.

lance6716 · 2024-04-25T10:12:03Z

/ok-to-test

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2024-04-25T10:50:44Z

/retest

codecov · 2024-04-25T10:56:37Z

Codecov Report

Attention: Patch coverage is 53.36927% with 173 lines in your changes are missing coverage. Please review.

Project coverage is 55.2474%. Comparing base (e8c397f) to head (00cd2df).
Report is 7 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #52885         +/-   ##
=================================================
- Coverage   72.4341%   55.2474%   -17.1867%     
=================================================
  Files          1481       1593        +112     
  Lines        428353     598449     +170096     
=================================================
+ Hits         310274     330628      +20354     
- Misses        98884     244842     +145958     
- Partials      19195      22979       +3784

Flag	Coverage Δ
integration	`22.1066% <30.4582%> (?)`
unit	`71.2035% <49.8652%> (-0.1149%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`49.7859% <ø> (+8.5684%)`	⬆️

D3Hunter

rest lgtm

D3Hunter · 2024-04-25T11:04:15Z

pkg/ddl/ingest/env.go

 	"github.com/pingcap/tidb/pkg/util/memory"
 	"github.com/pingcap/tidb/pkg/util/size"
 	"go.uber.org/zap"
 )

 var (
-	litLogger = logutil.BgLogger().With(zap.String(logutil.LogFieldCategory, "ddl-ingest"))
+	// LitLogger is the package-level logger.
+	LitLogger = zap.NewNop()


leave previous value? so we can see logs in test

In addition, not using a uniform initialization log interface may miss some configurations.

leave previous value? so we can see logs in test

In fact I think printing all message of passed unit test is useless, makes me hard to locate the log of failed tests. I'll try to initialize these logger to https://pkg.go.dev/go.uber.org/zap/zaptest#NewLogger

leave previous value? so we can see logs in test

In fact I think printing all message of passed unit test is useless, makes me hard to locate the log of failed tests. I'll try to initialize these logger to https://pkg.go.dev/go.uber.org/zap/zaptest#NewLogger

I'll revert to previous values, it's hard to write a helper function and use it once to get testing.T for zaptest.NewLogger

I found https://github.com/pingcap/tidb/blob/master/pkg/statistics/handle/logutil/logutil.go

Do you think this style is good? The SamplerLogger can also replace our manually down-sample logging logic

tidb/pkg/ddl/backfilling.go

Lines 279 to 280 in e8c397f

lastLogCount := 0

lastLogTime := time.Now()

cc @D3Hunter @zimulala

leave previous value? so we can see logs in test

In fact I think printing all message of passed unit test is useless, makes me hard to locate the log of failed tests. I'll try to initialize these logger to https://pkg.go.dev/go.uber.org/zap/zaptest#NewLogger

if the test is flaky, it might help, especially for cases that only trigger on some timeline, such as DXF

zaptest.NewLogger will print the content when test failed, or use go test -v.

pkg/ddl/syncer/syncer.go

pkg/ddl/ingest/env.go

Signed-off-by: lance6716 <lance6716@gmail.com>

Benjamin2037

LGTM

ti-chi-bot · 2024-04-26T06:07:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, D3Hunter

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:

~~OWNERS~~ [Benjamin2037,D3Hunter]
~~pkg/ddl/OWNERS~~ [Benjamin2037]

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

ti-chi-bot · 2024-04-26T06:07:49Z

[LGTM Timeline notifier]

Timeline:

2024-04-25 11:07:09.45381419 +0000 UTC m=+258386.193717114: ☑️ agreed by D3Hunter.
2024-04-26 06:07:48.354146692 +0000 UTC m=+326825.094049611: ☑️ agreed by Benjamin2037.

lance6716 · 2024-04-28T01:24:42Z

/cherry-pick release-8.1

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2024-04-28T01:25:25Z

@lance6716: new pull request created to branch release-8.1: #52942.

In response to this:

/cherry-pick release-8.1

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 ti-community-infra/tichi repository.

…52942) close #52883

ddl: use package-level logger which preset "category" field

dcc079f

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 25, 2024

ti-chi-bot bot added the ok-to-test label Apr 25, 2024

try fix CI

761bada

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Apr 25, 2024

fix bazel

e600941

Signed-off-by: lance6716 <lance6716@gmail.com>

D3Hunter approved these changes Apr 25, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label Apr 25, 2024

zimulala reviewed Apr 25, 2024

View reviewed changes

pkg/ddl/ingest/env.go Outdated Show resolved Hide resolved

lance6716 added 2 commits April 26, 2024 11:29

change logger usage to function

6857126

Signed-off-by: lance6716 <lance6716@gmail.com>

fix bazel

00cd2df

Signed-off-by: lance6716 <lance6716@gmail.com>

Benjamin2037 approved these changes Apr 26, 2024

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm labels Apr 26, 2024

ti-chi-bot bot merged commit 10971ea into pingcap:master Apr 26, 2024
23 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 28, 2024

This is an automated cherry-pick of pingcap#52885

113fee0

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Apr 28, 2024

ddl: use package-level logger which preset "category" field (#52885) #52942

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Apr 28, 2024

ddl: use package-level logger which preset "category" field (#52885) (#…

f0f7a4d

…52942) close #52883

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: use package-level logger which preset "category" field #52885

ddl: use package-level logger which preset "category" field #52885

lance6716 commented Apr 25, 2024 •

edited

tiprow bot commented Apr 25, 2024

lance6716 commented Apr 25, 2024

lance6716 commented Apr 25, 2024

codecov bot commented Apr 25, 2024 •

edited

D3Hunter left a comment

D3Hunter Apr 25, 2024

zimulala Apr 25, 2024

lance6716 Apr 25, 2024 •

edited

lance6716 Apr 25, 2024

lance6716 Apr 25, 2024

D3Hunter Apr 26, 2024

lance6716 Apr 26, 2024

Benjamin2037 left a comment

ti-chi-bot bot commented Apr 26, 2024

ti-chi-bot bot commented Apr 26, 2024

lance6716 commented Apr 28, 2024

ti-chi-bot commented Apr 28, 2024

ddl: use package-level logger which preset "category" field #52885

ddl: use package-level logger which preset "category" field #52885

Conversation

lance6716 commented Apr 25, 2024 • edited

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Apr 25, 2024

lance6716 commented Apr 25, 2024

lance6716 commented Apr 25, 2024

codecov bot commented Apr 25, 2024 • edited

Codecov Report

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Apr 25, 2024

Choose a reason for hiding this comment

zimulala Apr 25, 2024

Choose a reason for hiding this comment

lance6716 Apr 25, 2024 • edited

Choose a reason for hiding this comment

lance6716 Apr 25, 2024

Choose a reason for hiding this comment

lance6716 Apr 25, 2024

Choose a reason for hiding this comment

D3Hunter Apr 26, 2024

Choose a reason for hiding this comment

lance6716 Apr 26, 2024

Choose a reason for hiding this comment

Benjamin2037 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Apr 26, 2024

ti-chi-bot bot commented Apr 26, 2024

[LGTM Timeline notifier]

lance6716 commented Apr 28, 2024

ti-chi-bot commented Apr 28, 2024

lance6716 commented Apr 25, 2024 •

edited

codecov bot commented Apr 25, 2024 •

edited

lance6716 Apr 25, 2024 •

edited