Remove usage of InternalsVisibleTo in the internal instrumentations. #1856

ejsmith · 2021-02-26T18:56:23Z

Changes

This PR removes usage of InternalsVisibleTo within the included instrumentations. This makes it so that 3rd party instrumentations are working with the same API and can create their own instrumentations without being blocked by internal APIs. This change required adding a lot of linked source files to each instrumentation which would be nice if there weren't so many, but it does work and is something that 3rd party instrumentations can do and know that they are using unsupported APIs.

Doing this exposed that SuppressInstrumentationScope.IncrementIfTriggered and SuppressInstrumentationScope.DecrementIfTriggered need to be part of the public API.

Changes in public API reviewed

codecov · 2021-02-26T19:06:29Z

Codecov Report

Merging #1856 (09e5c93) into main (fb38dd8) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1856      +/-   ##
==========================================
- Coverage   83.31%   83.30%   -0.02%     
==========================================
  Files         188      188              
  Lines        6139     6139              
==========================================
- Hits         5115     5114       -1     
- Misses       1024     1025       +1

Impacted Files	Coverage Δ
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs	`86.91% <0.00%> (-2.81%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`82.35% <0.00%> (+5.88%)`	⬆️

src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj

reyang · 2021-02-26T19:24:37Z

Do we want the instrumentation libraries to take dependency on the SDK, or just the API?

In the initial PR #960, I suggested that we might want instrumentation library to take the convention rather than the dependency on the SDK. Happy to revisit this topic since the scope has changed (initially we didn't think instrumentation library should change the flag, now we do).

ejsmith · 2021-02-26T22:06:08Z

Just pushed an update to move the includes to the Common.props file which cleans things up pretty nicely.

Oberon00 · 2021-02-26T22:09:19Z

Do we want the instrumentation libraries to take dependency on the SDK, or just the API?

The spec requires that instrumentation libraries must be able to depend only on the API. Otherwise, there is no reason for the SDK/API split to exist. https://github.com/open-telemetry/opentelemetry-specification/blob/v1.0.1/specification/library-guidelines.md#requirements

Third party libraries and frameworks that add instrumentation to their code will have a dependency only on the API of OpenTelemetry client. The developers of third party libraries and frameworks do not care (and cannot know) what specific implementation of OpenTelemetry is used in the final application.

reyang · 2021-02-26T22:14:10Z

Do we want the instrumentation libraries to take dependency on the SDK, or just the API?

The spec requires that instrumentation libraries must be able to depend only on the API. Otherwise, there is no reason for the SDK/API split to exist. https://github.com/open-telemetry/opentelemetry-specification/blob/v1.0.1/specification/library-guidelines.md#requirements

Yep, the challenge here is that some instrumentation libraries want (not that they must) to take dependency on the SDK as they need additional features (e.g. "SuppressInstrumention" which is not an API concept). And I think it is not something I would encourage. So we might need to consider putting the "wanted" features to the API.

reyang · 2021-02-26T22:21:30Z

@Oberon00 I haven't been working on OTel Python for long time, do you know if there is a similar requirement in Python client, that an instrumentation library would want to suppress the spans from underlying library? #960

In OTel.NET, there is high demand on the following scenario:

Customer uses both gRPC and HTTP clients in their app (e.g. they make calls to service A which is gRPC, and to service B which is HTTP)
Customer is using gRPC lib + instrumentation library for gRPC calls.
Customer is using HttpClient + instrumentation library for HTTP calls.
gRPC client calls HttpClient internally, which is also instrumented automatically by the instrumentation library (mentioned in step 3).
Customer only wants to see the HttpClient calls that they made, rather than the calls made by gRPC client.

cijothomas

Thanks.

The issue about some instrumentations depending on SDK and not API is separately addressable (or not). Not blocking this PR.

reyang

LGTM.

cijothomas · 2021-02-26T22:48:59Z

I am modifying the section about "How to write instrumentation library", to reflect recent changes of eliminating ActivitySourceAdapter.
https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/trace/extending-the-sdk#instrumentation-library
Will mention about API vs SDK dependency there.
As of today, if instrumentation require supress-downsteam capability, it needs SDK, and it means this instrumentation is tied to this SDK implementation. We'll open an issue to see if API can support suppress feature.

reyang · 2021-02-26T22:52:19Z

This PR is related to #893 which has a dependency on open-telemetry/opentelemetry-specification#530 which points to https://github.com/lmolkova/oteps/blob/terminal-context/text/0069-terminal-context.md.

ejsmith · 2021-02-26T22:57:13Z

This PR is related to #893 which has a dependency on open-telemetry/opentelemetry-specification#530 which points to https://github.com/lmolkova/oteps/blob/terminal-context/text/0069-terminal-context.md.

I guess it relates to it in just that it is making 2 additional methods on SuppressInstrumentationScope public which were being used by the instrumentations in this project.

CodeBlanch · 2021-02-26T23:18:39Z

Hey just FYI there is also this thing IgnoresAccessChecksToAttribute which @mconnew brought up here:
open-telemetry/opentelemetry-dotnet-contrib#38 (comment)

I looked at it bit. Not much (zero?) documentation and it seems kind of heavy, but something else to consider.

ejsmith · 2021-02-27T06:41:52Z

Hey just FYI there is also this thing IgnoresAccessChecksToAttribute which @mconnew brought up here:

open-telemetry/opentelemetry-dotnet-contrib#38 (comment)

I looked at it bit. Not much (zero?) documentation and it seems kind of heavy, but something else to consider.

I can live with the tests using internals. I just want the community to be able to make their own instrumentation's the same way you all make the ones that are included in the project. This PR will make sure that we can do that.

src/OpenTelemetry/SuppressInstrumentationScope.cs

ejsmith · 2021-03-10T00:31:49Z

@cijothomas this is broken now because the instrumentations can't call the increment and decrement methods any more. I'm trying to see if I can just remove them from DiagnosticSourceListener, but that breaks the tests. Also, if I make it use the approach that I am doing from contrib where it's using it's own RuntimeContextSlot, it also breaks tests.

ejsmith · 2021-03-10T00:42:20Z

I pushed the change using the dedicated RuntimeContextSlot even though it's breaking tests. Hoping someone might know why it broke all the tests.

ejsmith · 2021-03-10T00:50:57Z

Oh fun. So this is one of those where tests fail when all run together, but if you run a small group at a time they pass. So I'm guessing that somehow the RuntimeContextSlot is being cross contaminated from other tests running at the same time which is obviously an issue. Any ideas as to why that might be?

cijothomas · 2021-03-10T01:02:17Z

I pushed the change using the dedicated RuntimeContextSlot even though it's breaking tests. Hoping someone might know why it broke all the tests.

#1892 I tried the same in standalone PR.

cijothomas · 2021-03-10T01:08:41Z

Oh fun. So this is one of those where tests fail when all run together, but if you run a small group at a time they pass. So I'm guessing that somehow the RuntimeContextSlot is being cross contaminated from other tests running at the same time which is obviously an issue. Any ideas as to why that might be?

I'll check.

ejsmith · 2021-03-10T01:31:35Z

So I guess the difference between your PR and mine is that I have a copy of the DiagnosticSourceListener in each instrumentation project and each having their own instance of RuntimeContextSlot pointing to the same named slot.

ejsmith · 2021-03-10T02:14:34Z

Changed to add a random suffix to the end of the slot name, but this is probably not a great solution as it would add a lot of AsyncLocal's

ejsmith · 2021-03-10T16:26:19Z

@cijothomas this has been updated to only remove the internalsvisibleto now. Hopefully we can get this merged now and ensure that no other cheats for the internal instrumentations creep into the library.

build/Common.props

...y.Instrumentation.StackExchangeRedis/OpenTelemetry.Instrumentation.StackExchangeRedis.csproj

cijothomas

ready to go!
Thanks for continuous effort in driving this cleanups!

ejsmith requested a review from a team February 26, 2021 18:56

ejsmith mentioned this pull request Feb 26, 2021

Upgrading Elasticsearch to OTel version 1.0.2 open-telemetry/opentelemetry-dotnet-contrib#83

Merged

CodeBlanch reviewed Feb 26, 2021

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj Outdated Show resolved Hide resolved

reyang reviewed Feb 26, 2021

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNet/OpenTelemetry.Instrumentation.AspNet.csproj Outdated Show resolved Hide resolved

cijothomas approved these changes Feb 26, 2021

View reviewed changes

reyang approved these changes Feb 26, 2021

View reviewed changes

CodeBlanch reviewed Feb 27, 2021

View reviewed changes

src/OpenTelemetry/SuppressInstrumentationScope.cs Outdated Show resolved Hide resolved

reyang self-requested a review March 2, 2021 04:31

ejsmith force-pushed the main branch 2 times, most recently from 90f3d87 to ec49356 Compare March 10, 2021 00:08

cijothomas approved these changes Mar 10, 2021

View reviewed changes

cijothomas mentioned this pull request Mar 10, 2021

Modify httpclient instrumentation to take advantage of sampling decision #1894

Merged

3 tasks

ejsmith force-pushed the main branch from d4c8588 to 990c8cd Compare March 10, 2021 04:27

ejsmith and others added 4 commits March 9, 2021 23:06

Remove usage of InternalsVisibleTo in the internal instrumentations.

62a5200

Move shared instrumentation code into Common.props

b5d45ca

Update Common.props

e1a3a5e

Remove DecrementIfTriggered and IncrementIfTriggered from public API

86d2444

ejsmith force-pushed the main branch from 74950a8 to 86d2444 Compare March 10, 2021 05:08

Merge branch 'main' into main

4fb4c96

cijothomas reviewed Mar 10, 2021

View reviewed changes

build/Common.props Outdated Show resolved Hide resolved

Split up into 2 includes for instrumentations

fc5ef3c

cijothomas reviewed Mar 10, 2021

View reviewed changes

...y.Instrumentation.StackExchangeRedis/OpenTelemetry.Instrumentation.StackExchangeRedis.csproj Outdated Show resolved Hide resolved

Remove SDK reference

09e5c93

cijothomas approved these changes Mar 10, 2021

View reviewed changes

cijothomas merged commit f2266a6 into open-telemetry:main Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of InternalsVisibleTo in the internal instrumentations. #1856

Remove usage of InternalsVisibleTo in the internal instrumentations. #1856

ejsmith commented Feb 26, 2021

codecov bot commented Feb 26, 2021 •

edited

Loading

reyang commented Feb 26, 2021 •

edited

Loading

ejsmith commented Feb 26, 2021

Oberon00 commented Feb 26, 2021

reyang commented Feb 26, 2021

reyang commented Feb 26, 2021 •

edited

Loading

cijothomas left a comment

reyang left a comment

cijothomas commented Feb 26, 2021

reyang commented Feb 26, 2021

ejsmith commented Feb 26, 2021

CodeBlanch commented Feb 26, 2021

ejsmith commented Feb 27, 2021

ejsmith commented Mar 10, 2021 •

edited

Loading

ejsmith commented Mar 10, 2021

ejsmith commented Mar 10, 2021

cijothomas commented Mar 10, 2021

cijothomas commented Mar 10, 2021

ejsmith commented Mar 10, 2021

ejsmith commented Mar 10, 2021

ejsmith commented Mar 10, 2021

cijothomas left a comment

Remove usage of InternalsVisibleTo in the internal instrumentations. #1856

Remove usage of InternalsVisibleTo in the internal instrumentations. #1856

Conversation

ejsmith commented Feb 26, 2021

Changes

codecov bot commented Feb 26, 2021 • edited Loading

Codecov Report

reyang commented Feb 26, 2021 • edited Loading

ejsmith commented Feb 26, 2021

Oberon00 commented Feb 26, 2021

reyang commented Feb 26, 2021

reyang commented Feb 26, 2021 • edited Loading

cijothomas left a comment

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

cijothomas commented Feb 26, 2021

reyang commented Feb 26, 2021

ejsmith commented Feb 26, 2021

CodeBlanch commented Feb 26, 2021

ejsmith commented Feb 27, 2021

ejsmith commented Mar 10, 2021 • edited Loading

ejsmith commented Mar 10, 2021

ejsmith commented Mar 10, 2021

cijothomas commented Mar 10, 2021

cijothomas commented Mar 10, 2021

ejsmith commented Mar 10, 2021

ejsmith commented Mar 10, 2021

ejsmith commented Mar 10, 2021

cijothomas left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2021 •

edited

Loading

reyang commented Feb 26, 2021 •

edited

Loading

reyang commented Feb 26, 2021 •

edited

Loading

ejsmith commented Mar 10, 2021 •

edited

Loading