-
Notifications
You must be signed in to change notification settings - Fork 218
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 #1417 Change socket mode ping/pong from debug to trace #1421
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1421 +/- ##
============================================
- Coverage 73.26% 73.16% -0.10%
+ Complexity 4370 4367 -3
============================================
Files 474 474
Lines 14185 14185
Branches 1433 1433
============================================
- Hits 10393 10379 -14
- Misses 2947 2962 +15
+ Partials 845 844 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 💯
Not sure why the CI is failing 🤔
Pls also include SocketModeClient::initializeSessionMonitorExecutor() |
if (getLogger().isTraceEnabled()) { | ||
getLogger().trace("Failed to receive a pong message"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this a warning.
if (getLogger().isTraceEnabled()) { | |
getLogger().trace("Failed to receive a pong message"); | |
} | |
getLogger().warn("Failed to receive a pong message"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When taking time until successfully connecting, having these at WARN level every 100 milliseconds can be noisy, so we won't change the log level here.
if (getLogger().isTraceEnabled()) { | ||
getLogger().trace("Failed to receive a pong message: {}", ping); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this a warning.
if (getLogger().isTraceEnabled()) { | |
getLogger().trace("Failed to receive a pong message: {}", ping); | |
} | |
getLogger().warn("Failed to receive a pong message: {}", ping); |
CI is failing on code coverage because trace log level is not enabled. |
The codecov warning is because there is no test addition with trace-level logging, but I believe it's okay to ignore this time.
Good catch; I will update this PR later |
bdf235d
to
38344f5
Compare
This pull request resolves #1417
Category (place an
x
in each of the[ ]
)Requirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.