*: Combine LOG_XXX and LOG_FMT_XXX #5512

fuzhe1989 · 2022-07-30T14:02:22Z

Signed-off-by: fuzhe1989 fuzhe1989@gmail.com

What problem does this PR solve?

Issue Number: ref #5511

Problem Summary:

What is changed and how it works?

Overload macros by arg count.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

None

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

ti-chi-bot · 2022-07-30T14:02:23Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
ywqzzy

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

fuzhe1989 · 2022-07-30T14:05:34Z

Hi @JaySon-Huang, @zanmato1984, @solotzg could you please take a look at this pr at your earliest convenient time?

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

fuzhe1989 · 2022-07-30T14:09:32Z

/run-all-tests

sre-bot · 2022-07-30T14:33:33Z

Coverage for changed files

Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
logger_useful.h                    10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Files which contain no functions:
MacroUtils.h                        0                 0         -           0                 0         -           0                 0         -           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                              10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18924      9440             50.12%    213890  95337        55.43%

full coverage report (for internal network access only)

fuzhe1989 · 2022-08-01T02:21:04Z

/run-all-tests

fuzhe1989 · 2022-08-01T02:25:11Z

/run-all-tests

fuzhe1989 · 2022-08-01T02:36:24Z

/rebuild

sre-bot · 2022-08-01T02:52:10Z

Coverage for changed files

Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
logger_useful.h                    10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Files which contain no functions:
MacroUtils.h                        0                 0         -           0                 0         -           0                 0         -           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                              10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18924      9440             50.12%    213890  95348        55.42%

full coverage report (for internal network access only)

ywqzzy · 2022-08-01T03:23:58Z

/rebuild

fuzhe1989 · 2022-08-01T03:28:11Z

/rebuild

fuzhe1989 · 2022-08-01T03:31:39Z

/rebuild

fuzhe1989 · 2022-08-01T04:42:56Z

/rebuild

fuzhe1989 · 2022-08-01T06:27:54Z

/rebuild

fuzhe1989 · 2022-08-01T06:59:01Z

/rebuild

fuzhe1989 · 2022-08-01T07:50:12Z

/rebuild

fuzhe1989 · 2022-08-01T12:34:58Z

/rebuild

fuzhe1989 · 2022-08-01T12:38:14Z

/rebuild

fuzhe1989 · 2022-08-01T13:05:28Z

/rebuild

JaySon-Huang · 2022-08-01T13:42:30Z

/rebuild

fuzhe1989 · 2022-08-02T00:11:43Z

/rebuild

ywqzzy · 2022-08-02T00:26:55Z

/rebuild

fuzhe1989 · 2022-08-02T02:10:27Z

/run-all-tests

ti-chi-bot · 2022-08-02T02:13:15Z

@leiysky: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

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.

JaySon-Huang · 2022-08-02T02:28:52Z

/run-all-tests

ywqzzy · 2022-08-02T02:35:24Z

libs/libcommon/include/common/logger_useful.h

+        }                                                                                     \
+    } while (false)
+
+#define LOG_IMPL_CHOSER(...) TF_GET_29TH_ARG(__VA_ARGS__, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_1, LOG_IMPL_0)


Could you tell me why have 29 args？

Because in our repo the maximum of arguments count is 26.

sre-bot · 2022-08-02T02:41:27Z

Coverage for changed files

Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
logger_useful.h                    10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Files which contain no functions:
MacroUtils.h                        0                 0         -           0                 0         -           0                 0         -           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                              10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18924      9440             50.12%    213890  95346        55.42%

full coverage report (for internal network access only)

ywqzzy

LGTM

JaySon-Huang

lgtm

JaySon-Huang · 2022-08-02T02:59:52Z

/merge

ti-chi-bot · 2022-08-02T02:59:53Z

@JaySon-Huang: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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.

ti-chi-bot · 2022-08-02T02:59:54Z

This pull request has been accepted and is ready to merge.

Commit hash: 164c473

sre-bot · 2022-08-02T03:12:23Z

Coverage for changed files

Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
logger_useful.h                    10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Files which contain no functions:
MacroUtils.h                        0                 0         -           0                 0         -           0                 0         -           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                              10                 0   100.00%           2                 0   100.00%           9                 3    66.67%           6                 0   100.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18924      9440             50.12%    213890  95373        55.41%

full coverage report (for internal network access only)

ref pingcap#5511

breezewish · 2022-09-03T13:55:09Z

libs/libcommon/include/common/logger_useful.h

+#define LOG_IMPL(logger, PRIORITY, ...) LOG_IMPL_CHOSER(__VA_ARGS__)(logger, PRIORITY, __VA_ARGS__)
+// clang-format on
+
+#define LOG_TRACE(logger, ...) LOG_IMPL(logger, Poco::Message::PRIO_TRACE, __VA_ARGS__)


According to the doc:

A Message can also contain any number of named parameters that contain additional information about the event that caused the message.

Seems that we can even consider supporting named arguments (instead of fmt differently) in future?

cc @JaySon-Huang

Does "named arguments" means a behavior like this?

int val = 12345; LOG_DEBUG(log, "log some value"; val); // generate a log "log some value [val=12345]"

As Poco::Message copy the key, value to a map<string, string> then put it into the format buffer. I'm wondering whether it will bring performance regression for logging. 🤔

spdlog may be a good alternative considering performance. It also has many convenient features.

update

07adb49

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

ti-chi-bot added the release-note-none label Jul 30, 2022

fuzhe1989 requested a review from JaySon-Huang July 30, 2022 14:02

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 30, 2022

fuzhe1989 requested review from zanmato1984 and solotzg July 30, 2022 14:03

fuzhe1989 added 2 commits July 30, 2022 22:08

update

f474f1d

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

Merge branch 'master' into fuzhe/variadic_macro

b6fb81f

Update MacroUtils.h

47c753f

Update logger_useful.h

d0823c1

Update logger_useful.h

8893e52

Update logger_useful.h

2c3f240

Merge branch 'master' into fuzhe/variadic_macro

164c473

leiysky approved these changes Aug 2, 2022

View reviewed changes

ywqzzy reviewed Aug 2, 2022

View reviewed changes

ywqzzy approved these changes Aug 2, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 2, 2022

JaySon-Huang approved these changes Aug 2, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 2, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 2, 2022

ti-chi-bot merged commit bebd45a into pingcap:master Aug 2, 2022

fuzhe1989 deleted the fuzhe/variadic_macro branch August 2, 2022 03:28

JaySon-Huang pushed a commit to ti-chi-bot/tiflash that referenced this pull request Aug 31, 2022

*: Combine LOG_XXX and LOG_FMT_XXX (pingcap#5512)

30171bc

ref pingcap#5511

breezewish reviewed Sep 3, 2022

View reviewed changes

*: Combine LOG_XXX and LOG_FMT_XXX #5512

*: Combine LOG_XXX and LOG_FMT_XXX #5512

Conversation

fuzhe1989 commented Jul 30, 2022 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 30, 2022 • edited

fuzhe1989 commented Jul 30, 2022

fuzhe1989 commented Jul 30, 2022

sre-bot commented Jul 30, 2022

Coverage for changed files

Coverage summary

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

sre-bot commented Aug 1, 2022

Coverage for changed files

Coverage summary

ywqzzy commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

fuzhe1989 commented Aug 1, 2022

JaySon-Huang commented Aug 1, 2022

fuzhe1989 commented Aug 2, 2022

ywqzzy commented Aug 2, 2022

fuzhe1989 commented Aug 2, 2022

ti-chi-bot commented Aug 2, 2022

JaySon-Huang commented Aug 2, 2022

ywqzzy Aug 2, 2022

Choose a reason for hiding this comment

fuzhe1989 Aug 2, 2022

Choose a reason for hiding this comment

sre-bot commented Aug 2, 2022

Coverage for changed files

Coverage summary

ywqzzy left a comment

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

JaySon-Huang commented Aug 2, 2022

ti-chi-bot commented Aug 2, 2022

ti-chi-bot commented Aug 2, 2022

sre-bot commented Aug 2, 2022

Coverage for changed files

Coverage summary

breezewish Sep 3, 2022 • edited

Choose a reason for hiding this comment

JaySon-Huang Sep 5, 2022

Choose a reason for hiding this comment

fuzhe1989 Sep 5, 2022

Choose a reason for hiding this comment

fuzhe1989 commented Jul 30, 2022 •

edited

ti-chi-bot commented Jul 30, 2022 •

edited

breezewish Sep 3, 2022 •

edited