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

ssl_logger improvements #5790

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Conversation

Flameeyes
Copy link
Contributor

@Flameeyes Flameeyes commented Mar 15, 2022

This includes a small change to fill in the location for all ssl_logger users.

maybe_client_warn_no_verify was the only caller that was missing location metadata.

This is the only ssl_logger:log/4 call that passed an empty metadata
rather than providing at least the location.
@CLAassistant
Copy link

CLAassistant commented Mar 15, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2022

CT Test Results

       2 files       63 suites   43m 1s ⏱️
   702 tests    619 ✔️   82 💤 1
3 436 runs  2 595 ✔️ 841 💤 0

For more details on these failures, see this check.

Results for commit 7f3448d.

♻️ 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

@rickard-green rickard-green added the team:PS Assigned to OTP team PS label Mar 21, 2022
@IngelaAndin
Copy link
Contributor

This change is not as small as it may seem. This would make the notice and warning alerts of the ssl application disappear from the default log. The messages that today uses the otp/ssl domain are debugging messages that will be handled by the ssl applications own logger handler. To be able to group all ssl logs under the same top domain sounds like something that could be desirable, but would probably require some new logger application support and some new subdomains in ssl. We will consider implementing such a possibility for the future, but we can not include this PR as it is right now.

@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Mar 22, 2022
@Flameeyes
Copy link
Contributor Author

@IngelaAndin what about the first of the two commits (providing location for maybe_client_warn_no_verify)?

The problem right now is that there s no way to select logs coming from the SSL app, because that one source does not have even module information :/

@IngelaAndin
Copy link
Contributor

IngelaAndin commented Mar 22, 2022

The first commit could be added now. It's the domain part that is problematic. We have written an internal ticket to handle that part so if you want to change this PR to only provide the first commit we will consider it for inclusion in OTP-25

@Flameeyes
Copy link
Contributor Author

Done that, this is now a single commit pull request.

@IngelaAndin IngelaAndin removed the stalled waiting for input by the Erlang/OTP team label Mar 22, 2022
@IngelaAndin
Copy link
Contributor

I will put it into testing as soon as the next OTP release candidate is out (the PR just missed the deadline for that candidate but not too late for the release).

@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Mar 24, 2022
@IngelaAndin IngelaAndin merged commit cc65b85 into erlang:master Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS 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