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

Enable diagnostics in release #1263

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Rob-Hague
Copy link
Collaborator

This should make troubleshooting apps easier

Copy link
Member

@drieseng drieseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What rationele did you apply for deciding when to "protect" logging with a corresponding IsEnabled(...) check?

I see you protecting (cheap) warning-level logging, but not verbose-level logging. While you could expect more people to have warning and error level logging enabled by default, but almost no one enables verbose logging unless you're troubleshooting an issue.

Can you perform (and commit) a (a simple connect) benchmark that shows the impact on performance with all tracing disabled (with any without this PR applied)?

I don't think we have benchmarks where we make use of our docker infrastructure.
Would be nice to have this. @WojciechNagorski, am I right?

That said, I'd love to get this PR in! Thanks!

.editorconfig Outdated Show resolved Hide resolved
src/Renci.SshNet/Diagnostics.cs Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
src/Renci.SshNet/Security/KeyExchange.cs Show resolved Hide resolved
src/Renci.SshNet/Security/KeyExchange.cs Show resolved Hide resolved
src/Renci.SshNet/Security/KeyExchange.cs Show resolved Hide resolved
src/Renci.SshNet/Security/KeyExchange.cs Show resolved Hide resolved
src/Renci.SshNet/Security/KeyExchange.cs Show resolved Hide resolved
src/Renci.SshNet/Security/KeyExchange.cs Show resolved Hide resolved
@Rob-Hague
Copy link
Collaborator Author

What rationele did you apply for deciding when to "protect" logging with a corresponding IsEnabled(...) check?

If the message is a literal string, I did not guard it.

Can you perform (and commit) a (a simple connect) benchmark that shows the impact on performance with all tracing disabled (with any without this PR applied)?

I don't think we have benchmarks where we make use of our docker infrastructure.

I don't think a benchmark would be suited for such an end-to-end networking scenario (too much noise). The module shows as ~1% of CPU of a basic connect->listdirectory->downloadfiles scenario at verbose level, and does not show in a memory trace.

image

Where we could use the benchmarks project is to define a number of scenarios from which it is easy to generate profiles for identifying where we should place the focus on performance. I found a nice blog which could be used as a reference 😉

@WojciechNagorski WojciechNagorski self-assigned this Dec 15, 2023
@@ -554,9 +554,9 @@ protected virtual void Close()
// SSH_MSG_CHANNEL_CLOSE as response to a SSH_MSG_CHANNEL_CLOSE sent by the
// server
var closeWaitResult = _session.TryWait(_channelClosedWaitHandle, ConnectionInfo.ChannelCloseTimeout);
if (closeWaitResult != WaitResult.Success)
if (closeWaitResult != WaitResult.Success && Diagnostics.IsEnabled(TraceEventType.Warning))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why we need the condition Diagnostics.IsEnabled(TraceEventType.Warning) outside the log method.

what if someone forgot to check it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need it, it just prevents doing any extra work like allocating string instances unnecessarily in the case that the log level is not enabled. But TBH I don't think it would make a material impact either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants