Add Telemetry Consumption package #571

MihaZupan · 2020-11-24T19:40:26Z

Opening for feedback on the design and implementation.

Includes APIs for 6 EventSources:

Proxy (YARP)
Kestrel
Http
Sockets
NetSecurity
NameResolution

For each of those, providing 4 components:

I{Name}TelemetryConsumer - interface with methods mirroring EventSource's Events that are relevant within the scope of an HTTP request
{Name}Metrics - Immutable POCO containing properties mirroring EventSource's EventCounters
I{Name}MetricsConsumer - interface with a single method void On{Name}Metrics(Metrics oldMetrics, Metrics newMetrics)
Internal {Name}EventListenerService - internal infrastructure for dispatching events and metrics, exposed as an extension method on IReverseProxyBuilder: Add{Name}TelemetryListener

The added tests are not exhaustive for every event and parameter.

src/Microsoft.ReverseProxy.TelemetryConsumption/IReverseProxyBuilderTelemetryExtensions.cs

WeihanLi · 2020-11-27T15:48:13Z

src/ReverseProxy.TelemetryConsumption/Proxy/ProxyEventListenerService.cs

+                    return;
+                }
+
+                foreach (var consumer in _serviceProvider.GetServices<IProxyMetricsConsumer>())


Should we consider for consuming in parallel?

These callbacks execute inline within the request, so the expected scenario is to have very few (just 1) consumer and for it to be very cheap and execute quickly.
I don't believe we would gain anything from parallelizing consumers (and the cost of it would likely outweigh any benefit).

If you were only referring to the metrics part, we could consider pushing that work onto the ThreadPool instead

alnikola

Some comments.

alnikola · 2020-12-01T15:15:04Z

src/ReverseProxy.TelemetryConsumption/Kestrel/KestrelEventListenerService.cs

+
+            const int TotalEventCounters = 10;
+
+            if (++_eventCountersCount == TotalEventCounters)


Could you please move the increment out of if expression? It seems a bit unusual.

alnikola · 2020-12-01T15:17:48Z

src/ReverseProxy.TelemetryConsumption/Kestrel/KestrelEventListenerService.cs

+                    return;
+                }
+
+                foreach (var consumer in _serviceProvider.GetServices<IKestrelMetricsConsumer>())


If I understood it correctly, IKestrelMetricsConsumer will be resolved for each EventWrittenEventArgs which seems a lot. Would it be better to resolve it once and store on a field?

For the metrics, we're resolving the consumers on each EventCounters update - so once a second, which doesn't seem problematic.

For events, we are resolving it for each event.
Since we're relying on DI for lifetime management, we have to do it each time. If we documented that we disallow transient consumer services (since they don't make much sense to use for events anyway), we could avoid the cost on each event and instead use an AsyncLocal to cache the resolved services.

src/ReverseProxy.TelemetryConsumption/NameResolution/NameResolutionEventListenerService.cs

src/ReverseProxy.TelemetryConsumption/NameResolution/NameResolutionMetrics.cs

src/ReverseProxy.TelemetryConsumption/NetSecurity/INetSecurityMetricsConsumer.cs

src/ReverseProxy.TelemetryConsumption/NetSecurity/INetSecurityTelemetryConsumer.cs

MihaZupan · 2020-12-01T19:09:49Z

@davidni Could you please look at this as well to make sure the public APIs are matching your expectations/needs

src/ReverseProxy.TelemetryConsumption/Microsoft.ReverseProxy.Telemetry.Consumption.csproj

Tratcher · 2020-12-03T23:22:47Z

test/ReverseProxy.FunctionalTests/TelemetryConsumptionTests.cs

+            }
+        }
+
+        private sealed class MetricsConsumer :


How closely do these Telemetry and Metrics tests reflect the actual expected usage? If this isn't how we expect it to be used then please add a sample that shows the expected usage.

ManickaP

I commented only on the first set of consumer classes but the comments apply to all of them.

ManickaP · 2020-12-04T09:21:40Z

test/ReverseProxy.FunctionalTests/TelemetryConsumptionTests.cs

@@ -0,0 +1,257 @@
+// Copyright (c) Microsoft Corporation.


Since we're adding a new library ReverseProxy.TelemetryConsumption shouldn't the tests be also in their own test project?

src/ReverseProxy.TelemetryConsumption/Microsoft.ReverseProxy.Telemetry.Consumption.csproj

src/ReverseProxy.TelemetryConsumption/MetricsOptions.cs

src/ReverseProxy.TelemetryConsumption/Http/HttpEventListenerService.cs

ManickaP · 2020-12-04T14:46:24Z

src/ReverseProxy.TelemetryConsumption/Http/HttpEventListenerService.cs

+
+        protected override void OnEventWritten(EventWrittenEventArgs eventData)
+        {
+            if (eventData.EventId < 1)


If we throw an exception from OnEventWritten method, it'll bubble up into whatever place generated the event, e.g. the call of SendAsync in HttpProxy. Shouldn't the whole method body be in try catch block then? Or do we want the errors from events consumption affect the proxy and thus get noticed more easily?

src/ReverseProxy.TelemetryConsumption/Http/HttpEventListenerService.cs

ManickaP · 2020-12-04T15:55:26Z

test/ReverseProxy.FunctionalTests/TelemetryConsumptionTests.cs

+
+            var expected = new[]
+            {
+                "OnRequestStart-Kestrel",


Should these be created with nameof(...) as well? Or are they like this on purpose?

Most of these could be nameof, but some are explicitly different since some names repeat.

Specifically both Runtime Http and Kestrel include RequestStart and RequestStop events.

ManickaP · 2020-12-04T16:03:22Z

test/ReverseProxy.FunctionalTests/Common/TestEnvironment.cs

+
+namespace Microsoft.ReverseProxy.Common
+{
+    public class TestEnvironment


Thanks for extracting this.

shirhatti · 2020-12-08T17:54:54Z

src/ReverseProxy.TelemetryConsumption/Http/HttpEventListenerService.cs

+            }
+        }
+
+        protected override void OnEventWritten(EventWrittenEventArgs eventData)


OnEventWritten can run before the derived constructor on an EventListener runs.- dotnet/runtime#38804 (comment)

You may need to guard against it

See https://github.com/shirhatti/Metrics/blob/master/src/Microsoft.Extensions.Metrics/Listener.cs#L67 for an example

Added more checks to avoid this in d11d635.

From what I can tell, the linked example has a race condition:
After setting _initialized but before looping over the queue in the ctor, new EvnetSources may still be added to the queue and enabled in OnEventSourceCreated, and will thus be enabled twice (don't know if EventSource internals have any checks guarding against this).

EnableEvents is safe to call twice.

…rce ctor completes

Tratcher · 2020-12-15T18:06:10Z

src/ReverseProxy/Telemetry/ProxyStage.cs

@@ -3,7 +3,7 @@

 namespace Microsoft.ReverseProxy.Telemetry
 {
-    internal enum ProxyStage : int
+    public enum ProxyStage : int


I thought everything was supposed to be decoupled to the point where you didn't even need a dependency on the core package? Is this type the only dependency? Would duplicating it in the telemetry package work?

Yes, technically this is the only type we depend on.
As long as the values are kept in sync, we can duplicate it in the Telemetry package and remove a dependency on the core package entirely.

The only other thing the telemetry package depends on is that consumers of the library target a preview >= 7, otherwise they will not observe proxy events (wouldn't break anything tho).

MihaZupan · 2021-01-14T17:27:15Z

We're currently relying on the package reference in 3 places:

ProxyStage enum (currently internal in core package)
ProxyError enum (public)
IReverseProxyBuilder interface for the AddXTelemetryListener extension methods

The extension methods could be swapped to use IServiceCollection instead and the ProxyStage enum could be duplicated and only exposed in the Consumption package.

But we would have to duplicate the already public ProxyError enum.

I think we should:

keep the package reference to the core package
use the IReverseProxyBuilder interface
duplicate and expose the ProxyStage enum in the consumption package only
use the core package's ProxyError

@Tratcher thoughts?

Tratcher

That plan makes sense, but see my comment below about using IReverseProxyBuilder.

Tratcher · 2021-01-14T18:07:06Z

src/ReverseProxy.TelemetryConsumption/IReverseProxyBuilderTelemetryExtensions.cs

+            return builder;
+        }
+
+        public static IReverseProxyBuilder AddKestrelTelemetryListener(this IReverseProxyBuilder builder)


Kestrel, HttpClient, DNS, etc. specific extensions don't belong on IReverseProxyBuilder, move these to IServiceCollection. At that point I'm not sure if it makes sense to still have the others on IReverseProxyBuilder.

Changed all to use IServiceCollection instead.

MihaZupan · 2021-01-19T12:09:36Z

@Tratcher is there anything else that should be done here before merging the PR?

Tratcher

My only outstanding question is https://github.com/microsoft/reverse-proxy/pull/571/files#r535719286

MihaZupan · 2021-01-25T17:13:02Z

I added a sample telemetry & metrics consumer to Code.Sample. More examples can be added as part of #676.

I also wrapped the metrics consumers in a try/catch, passing any exceptions to ILogger instead of letting them propagate to the CounterGroup thread and crashing the process.

Add Telemetry Consumption package

6ce6fd0

MihaZupan requested review from ManickaP, Tratcher, davidfowl and alnikola November 24, 2020 19:41

davidfowl reviewed Nov 25, 2020

View reviewed changes

src/Microsoft.ReverseProxy.TelemetryConsumption/IReverseProxyBuilderTelemetryExtensions.cs Outdated Show resolved Hide resolved

MihaZupan added 3 commits November 27, 2020 16:15

Merge master

027601b

Remove Microsoft from name to match folder structure

4ac6e02

Remove redundant TryAddHostedService helper

8cde5cf

WeihanLi reviewed Nov 27, 2020

View reviewed changes

alnikola reviewed Dec 1, 2020

View reviewed changes

Tratcher requested review from davidni and shirhatti December 3, 2020 23:08

Tratcher reviewed Dec 3, 2020

View reviewed changes

ManickaP reviewed Dec 4, 2020

View reviewed changes

shirhatti reviewed Dec 8, 2020

View reviewed changes

MihaZupan added 5 commits December 15, 2020 13:47

Merge branch 'master' into telem-consumption

686d103

Guard against NREs when processing events before the derived EventSou…

d11d635

…rce ctor completes

Add MaxEventId check

372f519

Float => TimeSpan

9ec505a

Cross-target Core 3.1

03e5fc7

Tratcher reviewed Dec 15, 2020

View reviewed changes

Tratcher mentioned this pull request Jan 11, 2021

clustering of reverse proxy #648

Closed

Tratcher reviewed Jan 14, 2021

View reviewed changes

MihaZupan added 4 commits January 14, 2021 22:59

Duplicate ProxyStage and fix test

c6d32c7

Use IServiceCollection for adding Telemtry listeners

05e6d39

Add public API summary comments

145df1e

Fix copy-paste comment error

d7dd833

MihaZupan marked this pull request as ready for review January 18, 2021 17:16

MihaZupan mentioned this pull request Jan 19, 2021

Write Telemetry docs #676

Closed

Tratcher approved these changes Jan 19, 2021

View reviewed changes

MihaZupan added 2 commits January 25, 2021 12:50

Add sample Telemetry consumer to code sample

ce6fbbf

Catch and log exceptions from metric consumers

a0d7f40

MihaZupan added this to the YARP 1.0.0-preview9 milestone Jan 25, 2021

MihaZupan merged commit 33a175b into microsoft:master Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Telemetry Consumption package #571

Add Telemetry Consumption package #571

MihaZupan commented Nov 24, 2020

WeihanLi Nov 27, 2020

MihaZupan Nov 27, 2020 •

edited

Loading

alnikola left a comment

alnikola Dec 1, 2020

alnikola Dec 1, 2020

MihaZupan Dec 1, 2020

MihaZupan commented Dec 1, 2020

Tratcher Dec 3, 2020

ManickaP left a comment

ManickaP Dec 4, 2020

ManickaP Dec 4, 2020

ManickaP Dec 4, 2020

MihaZupan Dec 7, 2020

ManickaP Dec 4, 2020

shirhatti Dec 8, 2020

shirhatti Dec 8, 2020

MihaZupan Dec 15, 2020

shirhatti Jan 14, 2021

Tratcher Dec 15, 2020

MihaZupan Dec 15, 2020

MihaZupan commented Jan 14, 2021

Tratcher left a comment

Tratcher Jan 14, 2021

MihaZupan Jan 15, 2021

MihaZupan commented Jan 19, 2021

Tratcher left a comment

MihaZupan commented Jan 25, 2021


		const int TotalEventCounters = 10;

		if (++_eventCountersCount == TotalEventCounters)

Add Telemetry Consumption package #571

Add Telemetry Consumption package #571

Conversation

MihaZupan commented Nov 24, 2020

Choose a reason for hiding this comment

MihaZupan Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

alnikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MihaZupan commented Dec 1, 2020

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MihaZupan commented Jan 14, 2021

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MihaZupan commented Jan 19, 2021

Tratcher left a comment

Choose a reason for hiding this comment

MihaZupan commented Jan 25, 2021

MihaZupan Nov 27, 2020 •

edited

Loading