Instrumentation Adapters to support Activity API #701

cijothomas · 2020-05-26T21:43:43Z

Goal: Fixes 3-a,c,d,e,f from #684

An alternate proposal for #692
There are quite a number of TODOs part of this PR. I want to address them outside of this PR, as this PR already got too much with all the instrumentationadapters!. Want to fix all instrumentionadapters so as to find any issues with Activity API and give .NET Team feedback on the same.

This approach does the following:
Adds builder.AddActivitySource(string.Empty) to get callbacks for Activities created by old instrumentations. (they do new Activity(), instead of using ActivitySource). The ActivitySource will be empty for the activities created using old approach, and hence string.Empty is subscribed to get them.

Uses the same activity created by existing instrumentation, and adds tags to it by reading from payload.
The instrumentation now makes calls to Sampler, and set IsAllDataRequested and TraceFlags on Activity appropriately.

Compared to #692 , this avoids creating a new duplicate Activity. But this now forces Instrumentation to have dependency on the SDK, to do sampling.

Major TODOs I'll address following this PR:

Kind will be settable once .NET ships new build. This PR currently uses reflection.
Avoid instrumentation being dependent on Sampling - its currently hardcoded. We are exploring if .NET can expose a method to run GetRequestedDataUsingContext pipeline (this is where we attach sampling logic) for activities created outside ActivitySource. If this happens, we'll simply switch to it. If not, we'll expose similar in OP.API, so instrumentation simply calls this method which internally can determine sampling. (Exact details to be designed as followup)
Subscribing to ActivitySource String.Empty is probably too noisy. Need to design an alternative. Its possible to NOT subscribe to old Activity using ActivitySource, and instead make Instrumentation call the Start/Stop (or equivalent). (Exact details to be designed as followup)
Support ITextFormat for ActivityContext. This PR simply ignores this code path now.
Find a way to do filters. The current approach of httpclient/aspnet/core Filter mechanism is broken with this PR.
AzureClient instrumentation - kept it currently, but this will be moved outside of this repo.
Move the diagnosticsource listener code from SDK to API, so Instrumentation can depend on it by just referring to the OT API. (This issue is already existing, but listing here as well)
Cleanup ListenerHandle - there is code copying activity tags into span etc - they need to be removed once Activity replaces Span.

(tests doing some of the above are disabled until actual code fix is done)

I will open issues to track these separately and submit follow ups.

…ance it.

tarekgh · 2020-05-26T22:08:38Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

            var request = context.Request;

            // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md
            var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
+            if (activity != null)


activity [](start = 16, length = 8)

is it possible the Activity can be null?

No. I didnt need the activity null check.

cijothomas · 2020-05-26T22:16:32Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -35,6 +36,7 @@ internal class HttpInListener : ListenerHandler
        private readonly PropertyFetcher beforeActionTemplateFetcher = new PropertyFetcher("Template");
        private readonly bool hostingSupportsW3C = false;
        private readonly AspNetCoreInstrumentationOptions options;
+        private readonly ActivitySampler sampler = new AlwaysOnActivitySampler();


hardcoded sampler for just prototyping.

cijothomas · 2020-05-26T22:17:31Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

+                activity.DisplayName = path;
+            }
+
+            var samplingDecision = this.sampler.ShouldSample(activity.Context, activity.TraceId, default, path, activity.Kind, activity.Tags, activity.Links);


This is the part which makes me not 100% convinved this approach is right. All instrumentationadatpers now need to know about Samplers (which is SDK concept).

Instead of simply checking if activity.IsAllDataRequested, its now first setting IsAllDataRequested by running sampling algorithm

…hub.com/cijothomas/opentelemetry-dotnet into cijothomas/activity3adapter1a_alternate

cijothomas · 2020-06-02T16:21:40Z

src/OpenTelemetry/Trace/Export/SimpleActivityProcessor.cs

@@ -41,6 +43,16 @@ public SimpleActivityProcessor(ActivityExporter exporter)
        /// <inheritdoc />
        public override void OnStart(Activity activity)
        {
+            // Perform sampling decision if source is empty.


this is not 100% correct, as the sampling decision is now made with the current ActivityKind (which would be Internal, the default value). Though none of the built-in Samplers makes use of ActivityKind, nothing prevents one from writing Sampler which takes Kind into consideration.

More better would be to "feed" this activity created outside of ActivitySource, into the ActivityListener's GetRequestedDataUsingContext pipeline. That can be done in the instrumentationadapter itself, after setting the Kind to correct one, (and possibly enrich activity with more tags/links/events etc. if needed)

Thanks @CodeBlanch for proposing the idea of "feeding" an external activity to the GetRequestedDataUsingContext pipeline.

Did you present the idea to .NET team? Any feedback?

@CodeBlanch we (the .NET team) will look at that to decide what is the best way can achieve this scenario. Thanks for your feedback, it is really helpful.

Given that making sampling decision in SDK piece is going to be incorrect (as we have incorrect Kind), I'll move the sampling logic into the InstrumentationAdapters. Of course this forces Instrumentation to have dependency on Sampling - but doing this to make progress and uncover any other potential issues.

If everything else is good, and the only remaining issue is Instrumentation being aware of Samplers - we'll try to get a new API in activity source to run the GetRequestedDataUsingContext.

Given that making sampling decision in SDK piece is going to be incorrect (as we have incorrect Kind)

Is this to set the Kind? reflection can work here till we expose the Kind setter.

We can't set the Kind to the correct one in the OT SDK as SDK doesn't know what is the actual kind. InstrumentationAdapter knows it, and it sets it.

src/OpenTelemetry/Trace/Export/SimpleActivityProcessor.cs

Make Asp.Net instrumentation work with Activity API

…it tests and this is not planned to be in this repo as well.

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

CodeBlanch · 2020-06-05T17:39:50Z

src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetryBuilderExtensions.cs

@@ -39,6 +39,8 @@ public static class OpenTelemetryBuilderExtensions
                throw new ArgumentNullException(nameof(builder));
            }

+            // Asp.Net Core is not instrumented with ActivitySource, hence


@cijothomas Question... if we listen for string.Empty in theory we'll get activities from a bunch of legacy DiagnosticSources flowing through our listener, do we need to filter out the stuff we don't care about somewhere?

You are right. This will be followed up.

…hub.com/cijothomas/opentelemetry-dotnet into cijothomas/activity3adapter1a_alternate

linux-foundation-easycla · 2020-06-08T19:11:01Z

The committers are authorized under a signed CLA.

✅ Cijo Thomas (831d712, 737f9b6, 9b840f0, 036a9bb, 5b836a4, 382d85f, f1e5108, 170ad87, 0832e71, 3b4cbb7, 19d138f, 121e4b5, 823fee6, c759d8a, 6df93de, 0d07263, 692b0dc, 7185426, 37071c5, b889a8c)

…bscriptions.

cijothomas · 2020-06-09T22:38:37Z

samples/Exporters/Web/Startup.cs

-            });
+            OpenTelemetrySdk.Default.EnableOpenTelemetry(
+                (builder) => builder.AddRequestInstrumentation().AddDependencyInstrumentation()
+                .UseJaegerActivityExporter(o =>


moved to jaeger as zipkin exporter with activity is not yet ready

pjanotti · 2020-06-09T22:43:33Z

samples/Exporters/Web/Controllers/WeatherForecastController.cs

        [HttpGet]
        public IEnumerable<WeatherForecast> Get()
        {
+            var res = httpClient.GetStringAsync("http://google.com").Result;


I assume this is only for tests and not part of the final merge.

Yes I can remove that. The testhttpClient example was doing something similar.

To provide a little history, when I was building the .NET Framework instrumentation I added in those calls so I could do integration testing, basically. Make sure incoming & outgoing requests would show up in Jaeger/Zipkin correctly. In the end it made the samples more interesting so I left them in. 🤷‍♂️

I see, what about adding a comment stating that it exists to also include outgoing on the sample?

Yes adding a comment.

pjanotti · 2020-06-09T22:44:29Z

samples/Exporters/Web/OpenTelemetry.Exporter.Web.csproj

    <ProjectReference Include="..\..\..\src\OpenTelemetry.Instrumentation.AspNetCore\OpenTelemetry.Instrumentation.AspNetCore.csproj" />
    <ProjectReference Include="..\..\..\src\OpenTelemetry.Instrumentation.Dependencies\OpenTelemetry.Instrumentation.Dependencies.csproj" />
-    <ProjectReference Include="..\..\..\src\OpenTelemetry.Exporter.Zipkin\OpenTelemetry.Exporter.Zipkin.csproj" />


I will create the Activity exporter for Zipkin this week or the next.

pjanotti · 2020-06-09T22:44:38Z

samples/Exporters/Web/appsettings.json

-    "Endpoint": "http://localhost:9411/api/v2/spans"
+  "Jaeger": {
+    "ServiceName": "jaeger-test",
+    "Host": "localhost",


Side note: it feels strange that we need to set Jaeger Host and Port to what it should be default values. Anyway, to be addressed in a separate PR.

The defaults look like they are in there:

opentelemetry-dotnet/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterOptions.cs

Lines 35 to 40 in c7333e4

public string AgentHost { get; set; } = "localhost";

/// <summary>

/// Gets or sets the Jaeger agent "compact thrift protocol" port. Default value: 6831.

/// </summary>

public int AgentPort { get; set; } = 6831;

But IMO it's still worthwhile to show how you can change them via settings in the samples.

it's still worthwhile to show how you can change them via settings in the samples.

Makes sense, perhaps we should comment where appropriate so the code setting (again) the default values don't get copied and pasted everywhere.

pjanotti · 2020-06-09T22:45:31Z

src/OpenTelemetry.Instrumentation.AspNet.Win/Implementation/HttpInListener.cs

-
-            TelemetrySpan span;
-            if (this.options.TextFormat is TraceContextFormat)
+            activity.DisplayName = path;


Due to the risk of high-cardinality on the path OTel spec suggests using the HTTP method instead. That said the path is much more useful in general and having an option to add it (more likely the absolute path) will be useful. There are other locations also using the path as DisplayName, but other correctly use the helper that gets the name per OTel spec. BTW, the link on the comment above is broken.

@pjanotti We updated the outgoing calls (#633) but it looks like we didn't do incoming.

Yes incoming requests were still using path as SpanName, hence I used it as activity.DisplayName.
This shouldn't block this PR, as we can (and should) address this separate from this PR.

CodeBlanch · 2020-06-10T18:32:10Z

src/OpenTelemetry/Trace/Configuration/OpenTelemetrySdk.cs

    {
+        public static OpenTelemetrySdk Default = new OpenTelemetrySdk();


My vote would be for:

public class OpenTelemetrySdk : IDisposable { private OpenTelemetrySdk() {} public static OpenTelemetrySdk EnableOpenTelemetry(Action<OpenTelemetryBuilder> configureOpenTelemetryBuilder) }

Usage: OpenTelemetrySdk.EnableOpenTelemetry(...)

Over:

public class OpenTelemetrySdk : IDisposable { public static OpenTelemetrySdk Default = new OpenTelemetrySdk(); private OpenTelemetrySdk() {} public OpenTelemetrySdk EnableOpenTelemetry(Action<OpenTelemetryBuilder> configureOpenTelemetryBuilder) }

Usage: OpenTelemetrySdk.Default.EnableOpenTelemetry(...)

Reason: The Default doesn't seem to add anything and it's confusing because it is IDisposable. When I first saw it I thought I had to call OpenTelemetrySdk.Default.Dispose() on shutdown. But really you need to dispose your instance, eg:

private OpenTelemetrySdk _OpenTelemetrySdk; public void OnAppStart() => _OpenTelemetrySdk = OpenTelemetrySdk.EnableOpenTelemetry(...); public void OnAppStop() => _OpenTelemetrySdk?.Dispose();

If we want to keep the default, the pattern should be like this?

public void OnAppStart() => OpenTelemetrySdk.Default.EnableOpenTelemetry(...); public void OnAppStop() => _OpenTelemetrySdk.Default.Dispose();

I'm not 100% sure but I tend to like your approach @CodeBlanch but I think we can address it in a separate PR/discussion.

@CodeBlanch shared another proposal which sounds very good! I will add it as a follow up PR.

pjanotti

@cijothomas we have some follow-ups but it is in a good point to merge.

use the same activity created by existing instrumentation. Simply enh…

831d712

…ance it.

tarekgh reviewed May 26, 2020

View reviewed changes

cijothomas commented May 26, 2020

View reviewed changes

cijothomas added 8 commits June 1, 2020 10:32

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

737f9b6

remove sampling from instrumentation

9b840f0

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

036a9bb

Merge branch 'cijothomas/activity3adapter1a_alternate' of https://git…

5b836a4

…hub.com/cijothomas/opentelemetry-dotnet into cijothomas/activity3adapter1a_alternate

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

382d85f

add asp.net core instrumentation

f1e5108

Merge branch 'cijothomas/activity3adapter1a_alternate' of https://git…

170ad87

…hub.com/cijothomas/opentelemetry-dotnet into cijothomas/activity3adapter1a_alternate

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

0832e71

cijothomas commented Jun 2, 2020

View reviewed changes

src/OpenTelemetry/Trace/Export/SimpleActivityProcessor.cs Outdated Show resolved Hide resolved

cijothomas added 8 commits June 2, 2020 15:04

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

3b4cbb7

move sampling to Instrumentation for now.

19d138f

Make Asp.Net instrumentation work with Activity API

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

121e4b5

Add httpClient .net core instrumentation

823fee6

Ad SqlClientInstrumentation

c759d8a

remove sqlclientinstrumentation from previous instrumentation

6df93de

Quick implementation for AzureClients - not validated as there are un…

0d07263

…it tests and this is not planned to be in this repo as well.

fix examples

692b0dc

CodeBlanch reviewed Jun 5, 2020

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs Show resolved Hide resolved

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

7185426

CodeBlanch reviewed Jun 5, 2020

View reviewed changes

cijothomas added 2 commits June 8, 2020 12:10

fix sampling flag

37071c5

Merge branch 'cijothomas/activity3adapter1a_alternate' of https://git…

b889a8c

…hub.com/cijothomas/opentelemetry-dotnet into cijothomas/activity3adapter1a_alternate

made sample app work with jaeger

8630782

cijothomas closed this Jun 8, 2020

cijothomas reopened this Jun 8, 2020

cijothomas added 9 commits June 8, 2020 15:00

Mark todos and fix AspNet tests

c4834ae

Fix asp.net core tests and mark TODOs

581cd1e

Add TODO for httpclient .net core test and fix test

7341c59

add todo and fix httpclient test

d9c9fff

add todos and fil sqlclienttests

514d89c

Make OpenTelemetrySDK disposable and take care of disposing all ds su…

f939361

…bscriptions.

Added OpenTelemetry.Default instead of static method

a220d9e

Merge branch 'master' into cijothomas/activity3adapter1a_alternate

99b91bf

conflict resolve

63dc16f

cijothomas marked this pull request as ready for review June 9, 2020 20:25

cijothomas requested review from bruno-garcia, discostu105, lmolkova, MikeGoldsmith, pjanotti and SergeyKanzhelev as code owners June 9, 2020 20:25

cijothomas changed the title ~~Asp.Net Core Requests Instrumentation - alternate approach reusing existing Activity~~ Instrumentation Adapters to support Activity API Jun 9, 2020

cijothomas commented Jun 9, 2020

View reviewed changes

AspNet, AspNetCore fix Dispose issue

11a175a

pjanotti reviewed Jun 9, 2020

View reviewed changes

CodeBlanch reviewed Jun 10, 2020

View reviewed changes

cijothomas added 2 commits June 10, 2020 12:28

conflict

efdb1d3

stylecop stuff lost i merge

3646fa1

pjanotti approved these changes Jun 11, 2020

View reviewed changes

cijothomas merged commit bb4480e into open-telemetry:master Jun 11, 2020

This was referenced Jun 12, 2020

Epic - Span API -> Activity API #684

Closed

Add ActivityContextFormat and propagation #724

Merged

Instrumentation Adapter fixes for activity. #730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrumentation Adapters to support Activity API #701

Instrumentation Adapters to support Activity API #701

cijothomas commented May 26, 2020 •

edited

tarekgh May 26, 2020

cijothomas May 26, 2020

cijothomas May 26, 2020

cijothomas May 26, 2020

cijothomas Jun 2, 2020

cijothomas Jun 2, 2020

CodeBlanch Jun 2, 2020

tarekgh Jun 2, 2020

cijothomas Jun 4, 2020

tarekgh Jun 4, 2020

cijothomas Jun 4, 2020

CodeBlanch Jun 5, 2020

cijothomas Jun 9, 2020

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

cijothomas Jun 9, 2020

pjanotti Jun 9, 2020

cijothomas Jun 10, 2020

CodeBlanch Jun 10, 2020

pjanotti Jun 10, 2020 •

edited

cijothomas Jun 11, 2020

pjanotti Jun 9, 2020

pjanotti Jun 9, 2020

CodeBlanch Jun 10, 2020

pjanotti Jun 10, 2020

pjanotti Jun 9, 2020

CodeBlanch Jun 10, 2020

cijothomas Jun 11, 2020

CodeBlanch Jun 10, 2020

pjanotti Jun 11, 2020

cijothomas Jun 11, 2020

pjanotti left a comment

	public string AgentHost { get; set; } = "localhost";

	/// <summary>
	/// Gets or sets the Jaeger agent "compact thrift protocol" port. Default value: 6831.
	/// </summary>
	public int AgentPort { get; set; } = 6831;

		{
		public static OpenTelemetrySdk Default = new OpenTelemetrySdk();

Instrumentation Adapters to support Activity API #701

Instrumentation Adapters to support Activity API #701

Conversation

cijothomas commented May 26, 2020 • edited

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

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jun 8, 2020 • edited

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

pjanotti Jun 10, 2020 • edited

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

pjanotti left a comment

Choose a reason for hiding this comment

cijothomas commented May 26, 2020 •

edited

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

pjanotti Jun 10, 2020 •

edited