Log KestrelServerOptions when tracing is enabled #34680

davidfowl · 2021-07-24T16:53:12Z

Dump configured values in a JSON blob and emit an event

{
  "AllowSynchronousIO": false,
  "AddServerHeader": true,
  "AllowAlternateSchemes": false,
  "AllowResponseHeaderCompression": true,
  "EnableAltSvc": false,
  "IsDevCertLoaded": true,
  "RequestHeaderEncodingSelector": "default",
  "ResponseHeaderEncodingSelector": "default",
  "Limits": {
    "KeepAliveTimeout": "00:02:10",
    "MaxConcurrentConnections": null,
    "MaxConcurrentUpgradedConnections": null,
    "MaxRequestBodySize": 30000000,
    "MaxRequestBufferSize": 1048576,
    "MaxRequestHeaderCount": 100,
    "MaxRequestHeadersTotalSize": 32768,
    "MaxRequestLineSize": 8192,
    "MaxResponseBufferSize": 65536,
    "MinRequestBodyDataRate": "Bytes per second: 240, Grace Period: 00:00:05",
    "MinResponseDataRate": "Bytes per second: 240, Grace Period: 00:00:05",
    "RequestHeadersTimeout": "00:00:30",
    "Http2": {
      "MaxStreamsPerConnection": 100,
      "HeaderTableSize": 4096,
      "MaxFrameSize": 16384,
      "MaxRequestHeaderFieldSize": 16384,
      "InitialConnectionWindowSize": 131072,
      "InitialStreamWindowSize": 98304,
      "KeepAlivePingDelay": "10675199.02:48:05.4775807",
      "KeepAlivePingTimeout": "00:00:20"
    },
    "Http3": {
      "HeaderTableSize": 0,
      "MaxRequestHeaderFieldSize": 16384
    }
  },
  "ListenOptions": [
    {
      "Address": "https://127.0.0.1:7030",
      "IsTls": true,
      "Protocols": "Http1AndHttp2"
    },
    {
      "Address": "https://[::1]:7030",
      "IsTls": true,
      "Protocols": "Http1AndHttp2"
    },
    {
      "Address": "http://127.0.0.1:5030",
      "IsTls": false,
      "Protocols": "Http1AndHttp2"
    },
    {
      "Address": "http://[::1]:5030",
      "IsTls": false,
      "Protocols": "Http1AndHttp2"
    }
  ]
}

The payload size is event source and LOH safe so >= 2K and 4K (with more listen options).

Contributes to #34578

- Dump configured values in a dictionary and emit an event with the pairs

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

JamesNK · 2021-07-24T18:42:20Z

EventSource only allows primitive values afaik. If you want to use eventsource then could could log each name/value in a separate call.

Alternatively, DiagnosticSource supports passing a dictionary.

davidfowl · 2021-07-24T20:17:15Z

@noahfalk did some testing and a dictionary worked the other day. But maybe only in narrow cases. I'll likely use JSON if tho doesn't work

- Added listen options - Added IsDevCertLoaded property - Mored RegisterOptions to the end of StartAsync

davidfowl · 2021-07-26T04:12:37Z

/azp run

azure-pipelines · 2021-07-26T04:12:54Z

Azure Pipelines successfully started running 2 pipeline(s).

davidfowl · 2021-07-26T05:16:16Z

/azp run

azure-pipelines · 2021-07-26T05:16:48Z

Azure Pipelines successfully started running 2 pipeline(s).

shirhatti · 2021-07-26T18:34:35Z

I'm a little worried about the options going out of sync with the event. Assuming we use this pattern of adding Serialize method everywhere maybe have an analyzer to detect that?

But besides that, looks good to me 👍🏾

Tratcher · 2021-07-26T18:42:52Z

IsDevCertLoaded is internal, do we want to log it?

davidfowl · 2021-07-26T18:49:39Z

I'm a little worried about the options going out of sync with the event. Assuming we use this pattern of adding Serialize method everywhere maybe have an analyzer to detect that?

Code reviewz! But a valid concern. We can add a comment to the file but yeah, an analyzer could probably detect it.

IsDevCertLoaded is internal, do we want to log it?

Seems useful for debugging.

davidfowl · 2021-07-27T00:04:05Z

I should probably add a test

src/Servers/Kestrel/Core/src/MinDataRate.cs

JamesNK · 2021-07-27T00:09:37Z

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelEventSource.cs

+        [NonEvent]
+        public void Configuration(KestrelServerOptions options)
+        {
+            var bufferWriter = new ArrayBufferWriter<byte>();


Is there an equivalent of checking log level here? Only want to write JSON if Informational and above events are being used.

This:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelEventSource.cs

Line 63 in e470d40

if (IsEnabled(EventLevel.Informational, EventKeywords.None))

This is checked in AddServerOptions.

The Configuration event is invoked from OnEventCommand and that path ideally would also test the level. If that level test was here it would make it easy to have it enforced uniformly. Currently Configuration() is also a public API so any other code could become a new caller in the future.

halter73 · 2021-07-27T01:05:16Z

"ListenOptions": []

Do we want to log the addresses configured by the IServerAddressesFeature when there are no ListenOptions and when IServerAddressesFeature.PreferHostingUrls is true? Otherwise this might be a little misleading.

davidfowl · 2021-07-27T01:44:13Z

I do but it feels weird to put it on the object. Maybe it's fine?

noahfalk · 2021-07-27T07:19:29Z

src/Servers/Kestrel/test/InMemory.FunctionalTests/EventSourceTests.cs

            Assert.Equal(Guid.Empty, connectionStop.RelatedActivityId);

            Assert.Equal(eventIndex, events.Count);
        }


Your choice in deciding how much you care, but if you want to you could write a unit test that deserializes the serialized JSON to see if the value round trips. Another option would be reflecting over all property names to confirm that they match the names observed in the JSON.

I will write a test that JSON deserializes

noahfalk

LGTM

Co-authored-by: James Newton-King <[email protected]>

davidfowl · 2021-07-29T07:01:13Z

Do we want to log the addresses configured by the IServerAddressesFeature when there are no ListenOptions and when IServerAddressesFeature.PreferHostingUrls is true? Otherwise this might be a little misleading.

Dumped out OptionsInUse (seems to work well).

Log KestrelServerOptions when tracing is enabled

ca0d749

- Dump configured values in a dictionary and emit an event with the pairs

davidfowl requested review from noahfalk and shirhatti July 24, 2021 16:53

ghost added the area-runtime label Jul 24, 2021

davidfowl requested review from JamesNK, Tratcher and halter73 July 24, 2021 16:54

davidfowl added the feature-kestrel label Jul 24, 2021

davidfowl commented Jul 24, 2021

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs Outdated Show resolved Hide resolved

davidfowl added 5 commits July 24, 2021 19:29

Use JSON

5bc8626

Use the Utf8JsonWriter to write configuration values

202fbe6

Added more content

33bf15b

- Added listen options - Added IsDevCertLoaded property - Mored RegisterOptions to the end of StartAsync

Remove server options from event source tracking in server stop

8415a20

Handle new events gracefully

e470d40

davidfowl marked this pull request as ready for review July 26, 2021 23:51

davidfowl requested a review from BrennanConroy as a code owner July 26, 2021 23:51

JamesNK reviewed Jul 27, 2021

View reviewed changes

noahfalk reviewed Jul 27, 2021

View reviewed changes

noahfalk approved these changes Jul 27, 2021

View reviewed changes

Update src/Servers/Kestrel/Core/src/MinDataRate.cs

a2afbf3

Co-authored-by: James Newton-King <[email protected]>

Tratcher approved these changes Jul 28, 2021

View reviewed changes

Print the OptionsInUse

e11a88f

davidfowl enabled auto-merge (squash) July 29, 2021 07:48

davidfowl merged commit 2ee3f4a into main Jul 29, 2021

davidfowl deleted the davidfowl/kestrel-options-event branch July 29, 2021 10:44

ghost added this to the 6.0-rc1 milestone Jul 29, 2021

davidfowl mentioned this pull request Jul 30, 2021

Check that the event source is enabled before dumping config #34879

Merged

davidfowl mentioned this pull request Aug 12, 2021

Additional Diagnostics #34578

Open

9 tasks

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023

Log KestrelServerOptions when tracing is enabled #34680

Log KestrelServerOptions when tracing is enabled #34680

Uh oh!

Conversation

davidfowl commented Jul 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JamesNK commented Jul 24, 2021

Uh oh!

davidfowl commented Jul 24, 2021

Uh oh!

davidfowl commented Jul 26, 2021

Uh oh!

azure-pipelines bot commented Jul 26, 2021

Uh oh!

davidfowl commented Jul 26, 2021

Uh oh!

azure-pipelines bot commented Jul 26, 2021

Uh oh!

shirhatti commented Jul 26, 2021

Uh oh!

Tratcher commented Jul 26, 2021

Uh oh!

davidfowl commented Jul 26, 2021

Uh oh!

davidfowl commented Jul 27, 2021

Uh oh!

Uh oh!

JamesNK Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

JamesNK Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

halter73 Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

noahfalk Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

halter73 commented Jul 27, 2021

Uh oh!

davidfowl commented Jul 27, 2021

Uh oh!

noahfalk Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

davidfowl Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

noahfalk left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Jul 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

davidfowl commented Jul 24, 2021 •

edited

Loading