Added options to HttpWebRequest instrumentation. by CodeBlanch · Pull Request #741 · open-telemetry/opentelemetry-dotnet

CodeBlanch · 2020-06-19T05:02:18Z

I have OT deployed in a couple of test environments. Users are complaining there are a bunch of spans showing up that shouldn't be. I tracked this down to a product the IT department deploys. It is instrumenting the code at runtime and calling its local service over Http to spit out data. There is no way currently in OT to easily filter out requests like these. There is a TODO in HttpClientInstrumentationOptions noting the desire to expose a filtering API.

Changes

I simplified the exposed "EventFilter" API in HttpClientInstrumentationOptions to something more user friendly.
I added dedicated options for HttpWebRequest instrumentation since the new "EventFilter" API now has a strongly-typed request object on it. Updated HttpWebRequestActivitySource to use the options to allow users to filter out requests.
Since HttpWebRequestActivitySource is now aware of its options, I restored the HttpFlavor toggle (off by default) and hooked up the ITextFormatActivity. To use TextFormat in there fully I had to add IsInjected to the interface.

Checklist

I ran Unit Tests locally.
Design discussion issue Filter activites proposals. #809.
Changes in public API reviewed.

linux-foundation-easycla · 2020-06-19T05:02:21Z

The committers are authorized under a signed CLA.

✅ Mikel Blanchard (d9acf38, e5739fe, e14d605)

SergeyKanzhelev · 2020-06-24T00:02:42Z

src/OpenTelemetry.Api/Context/Propagation/ITextFormatActivity.cs

+        /// <param name="carrier">Object to extract context from. Instance of this object will be passed to the getter.</param>
+        /// <param name="getter">Function that will return string value of a key with the specified name.</param>
+        /// <returns><see langword="true" /> if the carrier has been injected with an activity context.</returns>
+        bool IsInjected<T>(T carrier, Func<T, string, IEnumerable<string>> getter);


can Fields collection be used here? I'm fine having this method, I suggest to describe use case in a form of an issue on specification repo explaining the need for this.

@SergeyKanzhelev I don't think Fields will work alone to determine if a request is instrumented or not. The TraceContextFormatActivity for example has 2 Fields defined: traceparent and tracestate. A request is instrumented if it has traceparent, tracestate is optional. So not enough information in Fields for me to know which ones are required to make the determination.

Also, spec issue open: open-telemetry/opentelemetry-specification#674

SergeyKanzhelev · 2020-06-24T00:03:32Z

src/OpenTelemetry.Api/Context/Propagation/TraceContextFormatActivity.cs

+                var traceparentCollection = getter(carrier, TraceParent);
+
+                // There must be a single traceparent
+                return traceparentCollection != null && traceparentCollection.Count() == 1;


so IsInjected will return false if there are two of them?

looks counterintuitive

I mirrored what is done in the Extract logic 🤷‍♂️ Easy enough to change it to return true if at least one is found. Let me know if you want to change it just here, in both spots, leave as-it, or whatever.

SergeyKanzhelev · 2020-06-24T00:07:04Z

...nTelemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.netfx.cs

                activity.AddTag(SpanAttributeConstants.HttpUrlKey, request.RequestUri.OriginalString);
-                activity.AddTag(SpanAttributeConstants.HttpFlavorKey, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion));
+
+                if (Options.SetHttpFlavor == true)


Do we need a generic callback here to collect additional attributes?

Seems like it would be useful. After this PR I want to extend the options. I was thinking I would do an issue first with the proposed API so we can discuss, I'll add that on there?

codecov · 2020-07-08T15:42:31Z

Codecov Report

Merging #741 into master will decrease coverage by 0.09%.
The diff coverage is 43.85%.

@@            Coverage Diff             @@
##           master     #741      +/-   ##
==========================================
- Coverage   68.95%   68.85%   -0.10%     
==========================================
  Files         226      226              
  Lines        6825     6852      +27     
  Branches     1129     1144      +15     
==========================================
+ Hits         4706     4718      +12     
- Misses       1818     1829      +11     
- Partials      301      305       +4

Impacted Files	Coverage Δ
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`9.52% <0.00%> (+2.02%)`	⬆️
...entation.Dependencies/HttpClientInstrumentation.cs	`66.66% <0.00%> (+4.16%)`	⬆️
...ion.Dependencies/OpenTelemetryBuilderExtensions.cs	`69.56% <ø> (ø)`
src/OpenTelemetry/Context/Propagation/B3Format.cs	`65.78% <0.00%> (-10.75%)`	⬇️
.../Context/Propagation/TraceContextFormatActivity.cs	`51.28% <31.57%> (-3.82%)`	⬇️
...n.Dependencies/HttpClientInstrumentationOptions.cs	`100.00% <100.00%> (+14.28%)`	⬆️
...es/Implementation/HttpHandlerDiagnosticListener.cs	`66.66% <100.00%> (ø)`
...penTelemetry/Trace/Export/BatchingSpanProcessor.cs	`21.05% <0.00%> (-2.64%)`	⬇️
...elemetry/Trace/Export/BatchingActivityProcessor.cs	`73.25% <0.00%> (+2.32%)`	⬆️
... and 1 more

cijothomas · 2020-07-09T17:26:34Z

Added this as agenda to next SIG meeting to discuss:
https://docs.google.com/document/d/1yjjD6aBcLxlRazYrawukDgrhZMObwHARJbB9glWdHj8/edit#

Since this is exposing a new public API, i'd like to get opinions from others as well. Had offline discussions with @CodeBlanch as well.

CodeBlanch · 2020-07-09T20:25:52Z

@cijothomas Thanks! I have a conflict for the SIG on 7/14 so I added some notes/thoughts on the agenda below what you added on there.

…ove ahead with the other stuff while the API is reviewed.

…Blanch/opentelemetry-dotnet into httpinstrumentation-options

CodeBlanch · 2020-07-10T18:24:30Z

Update...

Had a chat with @cijothomas on Gitter. This PR changes the options API to expose filtering publically but it also does some other stuff. Like bringing .NET Framework Http client instrumentation into parity with .NET Core Http client, as far as the other options (TextFormat, HttpFlavor, etc.). I switched the filter back to internal. If we decide to move forward with that API, we can make it public in another PR. But the rest of the changes, would everyone please review?

@reyang @SergeyKanzhelev @pjanotti

…ceparent header directly.

…Blanch/opentelemetry-dotnet into httpinstrumentation-options

CodeBlanch · 2020-07-18T20:56:08Z

@cijothomas Put the filter back to public based on the discussion in #809. Merged in master, updated conflicts. Ready for review!

cijothomas · 2020-07-19T20:28:37Z

src/OpenTelemetry.Instrumentation.Dependencies/HttpClientInstrumentationOptions.cs

-                    return false;
-                }
+            // applicationinsights
+            if (originalString.StartsWith("https://dc.services.visualstudio") ||


I think we can remove this. Its leftover from old exporter

Is the old exporter used at all or is it end-of-life? The reason I ask is if we remove this, and anyone uses it through NuGet or the contrib repo or wherever, we still need this to prevent an infinite export loop. We can only remove once we have a solve for the "terminal span" thing. I think? We could ask users to manually filter via the public API if they use the azure exporter?

cijothomas · 2020-07-19T20:29:48Z

src/OpenTelemetry.Instrumentation.Dependencies/HttpClientInstrumentationOptions.cs

-                    request.RequestUri != null &&
-                    request.Method == HttpMethod.Post)
+                Uri requestUri;
+                if (requestMessage.Method == HttpMethod.Post && (requestUri = requestMessage.RequestUri) != null)


do we have to filter only Post requests?

That is only for the internal filter, which happens to be all POSTs. The user filter will fire for all methods.

There were GET requests made by application insights exporter as well. (though only once at startup).
Now that we expose public api to filter, do we still need to have zipkin specific one?

Interesting, didn't know about the Azure GETs. The "POST" rule has been part of the filter since I have been working on it, so I'm not sure why it was originally put in. You want me to open up to all methods? Are the GET URLs the same as the POST ones?

I think we still need the zipkin automatic filter unless you are thinking we can direct users to manually add that filter via the public API if they decide to use the ZipkinExporter?

The ZipkinExporter in-theory could use the same public API. But it would have to somehow find the instance of the options, which is in the Dependencies assembly it doesn't have a reference to. Then it would have to co-op the user's filter, if they added one. It could get messy. Prefer leaving as-is and solving in a generic way. The whole "terminal span" thing.

Yes agree to leave this for now.

cijothomas · 2020-07-19T20:38:21Z

src/OpenTelemetry.Instrumentation.Dependencies/HttpClientInstrumentation.cs

        public HttpClientInstrumentation(ActivitySourceAdapter activitySource, HttpClientInstrumentationOptions options)
        {
-            this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpHandlerDiagnosticListener(options, activitySource), options.EventFilter);
+            this.diagnosticSourceSubscriber = new DiagnosticSourceSubscriber(new HttpHandlerDiagnosticListener(options, activitySource), (activitySource, arg1, arg2) => options?.EventFilter(activitySource, arg1) ?? true);


can options be null ever?

I just double-checked, no can't be null. All the instrumentation classes are internal, only way to create them is through the builder which news up the options instance.

…Blanch/opentelemetry-dotnet into httpinstrumentation-options

CodeBlanch added 3 commits June 18, 2020 21:34

Added options to HttpWebRequest instrumentation.

d9acf38

Cleaned up some usings.

e5739fe

Added IsInjected to ITextFormatActivity.

e14d605

CodeBlanch requested a review from a team June 19, 2020 05:02

cijothomas added 2 commits June 22, 2020 23:08

Merge branch 'master' into httpinstrumentation-options

b688649

Merge branch 'master' into httpinstrumentation-options

2943c83

SergeyKanzhelev reviewed Jun 24, 2020

View reviewed changes

CodeBlanch mentioned this pull request Jun 26, 2020

Proposal - Propagators API: Add IsInjected open-telemetry/opentelemetry-specification#674

Closed

cijothomas and others added 3 commits June 26, 2020 09:37

Merge branch 'master' into httpinstrumentation-options

df5952f

Merge branch 'master' into httpinstrumentation-options

bdc6c24

Merging from master.

36469ec

Merge branch 'master' into httpinstrumentation-options

f1b587e

cijothomas mentioned this pull request Jul 10, 2020

Fix and disable HttpWebRequest test #797

Merged

CodeBlanch added 3 commits July 10, 2020 11:05

Merging latest from master.

77a67ea

Switched the Http instrumentation filter back to internal so we can m…

8f8839f

…ove ahead with the other stuff while the API is reviewed.

Merge branch 'httpinstrumentation-options' of https://github.com/Code…

ef9b2d0

…Blanch/opentelemetry-dotnet into httpinstrumentation-options

CodeBlanch and others added 4 commits July 10, 2020 12:46

Merge branch 'master' into httpinstrumentation-options

9e0ed87

Merge branch 'master' into httpinstrumentation-options

5c09f26

Updated HttpClient instrumentation to check IsInjected instead of tra…

90f4f1b

…ceparent header directly.

Merge branch 'httpinstrumentation-options' of https://github.com/Code…

51ce5df

…Blanch/opentelemetry-dotnet into httpinstrumentation-options

cijothomas mentioned this pull request Jul 14, 2020

Filter activites proposals. #809

Closed

reyang added the area:sdk:instrumentation label Jul 18, 2020

Merging latest from master.

9c065d2

FilterFunc is back to public. Removed ctors from http options.

9e7c6b2

Merge branch 'master' into httpinstrumentation-options

68dc803

cijothomas reviewed Jul 19, 2020

View reviewed changes

cijothomas approved these changes Jul 19, 2020

View reviewed changes

CodeBlanch added 2 commits July 19, 2020 15:41

Removed ctor that isn't needed.

20a4775

Merge branch 'httpinstrumentation-options' of https://github.com/Code…

b46cfab

…Blanch/opentelemetry-dotnet into httpinstrumentation-options

cijothomas merged commit c294cd7 into open-telemetry:master Jul 20, 2020

CodeBlanch deleted the httpinstrumentation-options branch July 20, 2020 02:33

Conversation

CodeBlanch commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Checklist

Uh oh!

linux-foundation-easycla bot commented Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

Uh oh!

Codecov Report

Uh oh!

cijothomas commented Jul 9, 2020

Uh oh!

CodeBlanch commented Jul 9, 2020

Uh oh!

CodeBlanch commented Jul 10, 2020

Uh oh!

CodeBlanch commented Jul 18, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CodeBlanch commented Jun 19, 2020 •

edited

Loading

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

edited

Loading

codecov bot commented Jul 8, 2020 •

edited

Loading