Add telemetry to System.Net.Security #40108

MihaZupan · 2020-07-29T23:06:26Z

Contributes to #37428

Adds HandshakeStart, HandshakeStop, HandshakeFailed events.

Adds counters:

tls-handshakes-per-second
total-tls-handshakes
current-tls-handshakes
failed-tls-handshakes
all-tls-sessions-open
tls10-sessions-open
tls11-sessions-open
tls12-sessions-open
tls13-sessions-open
all-tls-handshake-duration
tls10-handshake-duration
tls11-handshake-duration
tls12-handshake-duration
tls13-handshake-duration

ghost · 2020-07-29T23:06:29Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

MihaZupan · 2020-07-29T23:07:33Z

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

+                    descrs[0] = new EventData
+                    {
+                        DataPointer = (IntPtr)(&arg1),
+                        Size = sizeof(int) // EventSource defines bool as a 32-bit type


Is this the right way of using bools in event data?

See https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventProvider.cs#L867 for the right way to encode bool types. Also, I believe that initlocals is not set for this assembly, so you will need to explicitly set EventData.Reserved=0. A non-zero value will cause failures that will result in the events being corrupted.

Given that the input types are all supported types, you could delete this entire method and just let EventSource handle it for you. It's OK to do this, but there's the potential for more bugs here.

Also, given that these are

Also, I believe that initlocals is not set for this assembly, so you will need to explicitly set EventData.Reserved=0. A non-zero value will cause failures that will result in the events being corrupted.

Reserved is not visible outside of its assembly. Using new EventData will zero-init that field, regardless of SkipLocalsInit.

Given that the input types are all supported types, you could delete this entire method and just let EventSource handle it for you.

Considering these events are only fired 2/3x per connection, the perf overhead likely wouldn't matter.

That's a good point - you're using the constructor here, so you should be fine.

brianrob · 2020-07-30T00:01:00Z

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

+        {
+            Interlocked.Increment(ref _startedTlsHandshakes);
+
+            if (IsEnabled(EventLevel.Informational, EventKeywords.None))


You can safely remove the IsEnabled checks inside these functions. They will occur just inside of WriteEvent. Arguments have already been created, so there isn't much perf to save here, especially if WriteEvent gets inlined.

brianrob · 2020-07-30T00:10:23Z

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

+                    descrs[0] = new EventData
+                    {
+                        DataPointer = (IntPtr)(&arg1),
+                        Size = sizeof(int) // EventSource defines bool as a 32-bit type


See https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventProvider.cs#L867 for the right way to encode bool types. Also, I believe that initlocals is not set for this assembly, so you will need to explicitly set EventData.Reserved=0. A non-zero value will cause failures that will result in the events being corrupted.

Given that the input types are all supported types, you could delete this entire method and just let EventSource handle it for you. It's OK to do this, but there's the potential for more bugs here.

Also, given that these are

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

MihaZupan · 2020-08-10T14:02:02Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-10T14:02:16Z

Azure Pipelines successfully started running 1 pipeline(s).

wfurt · 2020-08-11T02:53:33Z

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

+        private PollingCounter? _totalTlsHandshakesCounter;
+        private PollingCounter? _currentTlsHandshakesCounter;
+        private PollingCounter? _failedTlsHandshakesCounter;
+        private PollingCounter? _connectionsOpenTls10Counter;


Do we need to keep this per protocol version? That is going hard to maintain IMHO. (and not covering old SSL)

As far as I know, counters can't carry other data, so I don't know if this can be simplified

The key part is if ween stats per protocol. If we do, we have no option IMHO. In that case it may be worth of adding deprecating Ssl2/3 for completeness. And perhaps add some comment somewhere to update this when next TLS is out. I did not pay much attention. but do we could http1 and http2 separate? From SslStream prospective there is really not that much difference.

You're suggesting we add counters for Ssl2/3 as well even tho they're deprecated?

And perhaps add some comment somewhere to update this when next TLS is out.

Will do, we can also add asserts for that.

but do we could http1 and http2 separate? From SslStream prospective there is really not that much difference.

Do you mean have separate counters for open connections depending on whether they are used for Http1/Http2?
Afaik SslStream doesn't have any such data right?

no it does not. I was wondering if HTTP counters are also separated by protocol versions.
I was wondering why somebody would want to track handshakes by protocol version.
The answer may be monitoring of very old or very new versions to manage deprecation and adoption.
If we don't add deprecated Ssl, than the counters will not add-up to total number for handshakes.
That may be fine if we document it or if we add some "other" bucket.

HTTP counters aren't split by protocol version, the RequestStart event does log the version however.

If we don't add deprecated Ssl, than the counters will not add-up to total number for handshakes.

There is currently no counter for "total-opened-sessions" (though we can add it as it seems useful), there are only per-protocol ones (excluding Ssl2, 3).

And perhaps add some comment somewhere to update this when next TLS is out.

I added an assert that should catch that

There is currently no counter for "total-opened-sessions"

I added all-sessions-open and all-handshake-duration counters.

My understanding is that in most cases, a user would have to manually opt-in to using Ssl2/Ssl3, so I would be okay with not adding counters for those. They are still tracked in the all-* counters, so documenting it seems better.

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

wfurt

generally looks good. I left few comments.

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

wfurt

LGTM.

MihaZupan · 2020-08-11T21:05:48Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-11T21:06:01Z

Azure Pipelines successfully started running 1 pipeline(s).

MihaZupan · 2020-08-12T05:03:50Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-12T05:04:03Z

Azure Pipelines successfully started running 1 pipeline(s).

noahfalk · 2020-08-13T07:22:39Z

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

+                    DisplayName = "Total TLS handshakes failed"
+                };
+
+                _sessionsOpenCounter ??= new PollingCounter("all-sessions-open", this, () => Interlocked.Read(ref _sessionsOpen))


Suggested change

_sessionsOpenCounter ??= new PollingCounter("all-sessions-open", this, () => Interlocked.Read(ref _sessionsOpen))

_sessionsOpenCounter ??= new PollingCounter("tls-sessions-open", this, () => Interlocked.Read(ref _sessionsOpen))

'all-sessions' seems ambiguous - I assume there are many other kinds of authenticated communication sessions even if .NET doesn't implement them (Kerberos, SSL, SSH?). Everywhere else we used 'tls' so can we use it here too?

Here it includes tls and ssl2, ssl3. (same for comments below).

Using tls in that case would be less misleading than all. I'm not aware of a different term that would be more appropriate here.

Using tls in that case would be less misleading than al

In context I am guessing you meant more misleading? : ) What about "TLS/SSL Sessions Active"?

What about "TLS/SSL Sessions Active"?

As the description, yes. I suppose that's enough clarification even if we use the tls-* event name.

tls-ssl-sessions-open?

That does drag along all "generic" counters:
tls-handshakes-per-second
total-tls-handshakes
current-tls-handshakes
failed-tls-handshakes

to
tls-ssl-handshakes-per-second
total-tls-ssl-handshakes
current-tls-ssl-handshakes
failed-tls-ssl-handshakes

@wfurt Do you have a preference here regarding "tls-" vs "tls-ssl-" vs "all-" naming?

I would use "tls-". We use either one but I have not seem them together like this. Possibly something like "all-tls-sessions-open".
To @noahfalk 's point. We may add some telemetry to NegotiatStream/Kerberos.

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs

MihaZupan · 2020-08-13T20:39:43Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-13T20:39:56Z

Azure Pipelines successfully started running 1 pipeline(s).

MihaZupan added 2 commits July 30, 2020 00:51

Add telemetry to System.Net.Security

7d2c733

Remove leftover test code

ea05684

MihaZupan added the area-System.Net.Security label Jul 29, 2020

MihaZupan added this to the 5.0.0 milestone Jul 29, 2020

MihaZupan requested review from brianrob, noahfalk, wfurt, alnikola and a team July 29, 2020 23:06

MihaZupan commented Jul 29, 2020

View reviewed changes

brianrob reviewed Jul 30, 2020

View reviewed changes

Merge master

605f7ec

MihaZupan force-pushed the ssl-telem branch from 6481475 to 605f7ec Compare August 10, 2020 11:56

MihaZupan mentioned this pull request Aug 10, 2020

Test failure: System.Net.Tests.HttpWebRequestTest_Async.GetResponseAsync_ServerNameNotInDns_ThrowsWebException #40606

Closed

wfurt reviewed Aug 11, 2020

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

wfurt reviewed Aug 11, 2020

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/NetSecurityTelemetry.cs Outdated Show resolved Hide resolved

MihaZupan added 4 commits August 11, 2020 13:54

Merge branch 'master' into ssl-telem

8d9fb6f

Add Assert to ensure new counters are added with new TLS versions

d62b56e

Add Telemetry.IsEnabled() check

ebff2a9

Rename opened connections to opened sessions

42709e2

MihaZupan force-pushed the ssl-telem branch from 50c9a6b to 42709e2 Compare August 11, 2020 13:58

Add all-sessions-open and all-handshake-duration counters

e1b2005

wfurt approved these changes Aug 11, 2020

View reviewed changes

MihaZupan closed this Aug 12, 2020

MihaZupan reopened this Aug 12, 2020

noahfalk reviewed Aug 13, 2020

View reviewed changes

Rename all- counters to all-tls-

b30c903

MihaZupan merged commit 47345cd into dotnet:master Aug 14, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

	_sessionsOpenCounter ??= new PollingCounter("all-sessions-open", this, () => Interlocked.Read(ref _sessionsOpen))
	_sessionsOpenCounter ??= new PollingCounter("tls-sessions-open", this, () => Interlocked.Read(ref _sessionsOpen))

Add telemetry to System.Net.Security #40108

Add telemetry to System.Net.Security #40108

Uh oh!

Conversation

MihaZupan commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MihaZupan commented Aug 10, 2020

Uh oh!

azure-pipelines bot commented Aug 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

MihaZupan commented Aug 11, 2020

Uh oh!

azure-pipelines bot commented Aug 11, 2020

Uh oh!

MihaZupan commented Aug 12, 2020

Uh oh!

azure-pipelines bot commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MihaZupan commented Aug 13, 2020

Uh oh!

azure-pipelines bot commented Aug 13, 2020

Uh oh!

Uh oh!

MihaZupan commented Jul 29, 2020 •

edited

Loading