Skip to content
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.

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

Fix logger_formatter about msg by making templates linear before split #6036

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

gfngfn
Copy link
Contributor

@gfngfn gfngfn commented May 31, 2022

I have encountered a somewhat strange behavior of logger_formatter about msg in templates. This pull request attempts to remedy it.

The behavior is exemplified by the following application:

In summary, msgs occurring at conditional branching {metakey(), template(), template()} in templates will not be formatted as intended.

Looking into the implementation of logger_formatter:format, I have found that the implementation presumably assumes that given templates are of the form [Elem_1, …, Elem_{i - 1}, msg, Elem_{i + 1}, …, Elem_n] or do not contain msg. According to the definition of logger_formatter:template(), this assumption is not documented. Therefore, one of the following amendments seems to be needed:

  1. State in the manual that msg must occur as a top level element of templates and at most once;
  2. Fix the implementation of logger_formatter:format so that msgs can occur under conditional branching but at most once for every “path”; or
  3. Fix the implementation of logger_formatter:format so that msgs can occur everywhere in templates possibly more than once.

The present pull request suggests an amendment based on the second approach. This is because using msg more than once in a “path” does not seem so usual and also its support will require a bit complicated implementation due to the chars_limit option.

I would appreciate it if you could give any comments or suggestions (especially since this is my first pull request to this repository).

@CLAassistant
Copy link

CLAassistant commented May 31, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

github-actions bot commented May 31, 2022

CT Test Results

       2 files       65 suites   53m 58s ⏱️
1 321 tests 1 181 ✔️ 140 💤 0
1 476 runs  1 305 ✔️ 171 💤 0

Results for commit fe36d8b.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

This modification is for fixing a strange behavior of the
logger formatter about `msg` occurring at conditional branching
in logger templates; see
https://github.com/gfngfn/a_strange_behavior_of_logger_formatter
for example.
@gfngfn gfngfn force-pushed the fix-logger-formatter-about-msg branch from 7502c1d to fe36d8b Compare June 2, 2022 17:29
@gfngfn
Copy link
Contributor Author

gfngfn commented Jun 2, 2022

Rebased the branch onto maint and pushed it with --force (because the CI workflow had failed probably due to some other cause than this pull request). The workflow seems passing in this time on the forked repository: https://github.com/gfngfn/otp/actions/runs/2429845241

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Jun 7, 2022
@garazdawi garazdawi self-assigned this Jun 8, 2022
@garazdawi garazdawi added this to the OTP-26.0 milestone Aug 19, 2022
@garazdawi garazdawi added testing currently being tested, tag is used by OTP internal CI bug Issue is reported as a bug labels Aug 19, 2022
@garazdawi garazdawi changed the base branch from maint to master August 19, 2022 07:32
@garazdawi
Copy link
Contributor

Thanks! I think the change looks good and makes sense. Putting it into testing to make sure it works on all OSs and configurations.

@garazdawi garazdawi merged commit 74cb43b into erlang:master Aug 26, 2022
@garazdawi
Copy link
Contributor

thanks!

@gfngfn
Copy link
Contributor Author

gfngfn commented Aug 27, 2022

Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants