-
Notifications
You must be signed in to change notification settings - Fork 773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SuppressInstrumentation from ActivityListener and DiagnosticSourceListener #1079
Merged
cijothomas
merged 7 commits into
open-telemetry:master
from
alanwest:alanwest/suppress-instrumentation
Aug 20, 2020
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
98b7327
Add SuppressInstrumentation check in the ActivityListener
alanwest 048e064
Add SuppressInstrumentation check in the DiagnosticSourceListner
alanwest 9521f87
Merge branch 'master' into alanwest/suppress-instrumentation
alanwest 7046974
Fix things post merge
alanwest d8203ed
Put AlwaysOn/AlwaysOff sampler check back in the constructor
alanwest 6b8ba03
Merge branch 'master' into alanwest/suppress-instrumentation
alanwest ceab2aa
Merge remote-tracking branch 'upstream/master' into alanwest/suppress…
alanwest File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60 changes: 60 additions & 0 deletions
60
test/OpenTelemetry.Tests/Instrumentation/DiagnosticSourceListenerTest.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
// <copyright file="DiagnosticSourceListenerTest.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
using System.Diagnostics; | ||
using Xunit; | ||
|
||
namespace OpenTelemetry.Instrumentation.Tests | ||
{ | ||
public class DiagnosticSourceListenerTest | ||
{ | ||
private const string TestSourceName = "TestSourceName"; | ||
private DiagnosticSource diagnosticSource; | ||
private TestListenerHandler testListenerHandler; | ||
private DiagnosticSourceSubscriber testDiagnosticSourceSubscriber; | ||
|
||
public DiagnosticSourceListenerTest() | ||
{ | ||
this.diagnosticSource = new DiagnosticListener(TestSourceName); | ||
this.testListenerHandler = new TestListenerHandler(TestSourceName); | ||
this.testDiagnosticSourceSubscriber = new DiagnosticSourceSubscriber(this.testListenerHandler, null); | ||
this.testDiagnosticSourceSubscriber.Subscribe(); | ||
} | ||
|
||
[Theory] | ||
[InlineData(true)] | ||
[InlineData(false)] | ||
public void ListenerHandlerIsNotInvokedWhenSuppressInstrumentationTrue(bool suppressInstrumentation) | ||
{ | ||
using var scope = SuppressInstrumentationScope.Begin(suppressInstrumentation); | ||
|
||
var activity = new Activity("Main"); | ||
this.diagnosticSource.StartActivity(activity, null); | ||
this.diagnosticSource.StopActivity(activity, null); | ||
|
||
if (suppressInstrumentation) | ||
{ | ||
Assert.Equal(0, this.testListenerHandler.OnStartInvokedCount); | ||
Assert.Equal(0, this.testListenerHandler.OnStopInvokedCount); | ||
} | ||
else | ||
{ | ||
Assert.Equal(1, this.testListenerHandler.OnStartInvokedCount); | ||
Assert.Equal(1, this.testListenerHandler.OnStopInvokedCount); | ||
} | ||
} | ||
} | ||
} |
53 changes: 53 additions & 0 deletions
53
test/OpenTelemetry.Tests/Instrumentation/TestListenerHandler.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
// <copyright file="TestListenerHandler.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
using System.Diagnostics; | ||
|
||
namespace OpenTelemetry.Instrumentation.Tests | ||
{ | ||
public class TestListenerHandler : ListenerHandler | ||
{ | ||
public int OnStartInvokedCount = 0; | ||
public int OnStopInvokedCount = 0; | ||
public int OnExceptionInvokedCount = 0; | ||
public int OnCustomInvokedCount = 0; | ||
|
||
public TestListenerHandler(string sourceName) | ||
: base(sourceName) | ||
{ | ||
} | ||
|
||
public override void OnStartActivity(Activity activity, object payload) | ||
{ | ||
this.OnStartInvokedCount++; | ||
} | ||
|
||
public override void OnStopActivity(Activity activity, object payload) | ||
{ | ||
this.OnStopInvokedCount++; | ||
} | ||
|
||
public override void OnException(Activity activity, object payload) | ||
{ | ||
this.OnExceptionInvokedCount++; | ||
} | ||
|
||
public override void OnCustom(string name, Activity activity, object payload) | ||
{ | ||
this.OnCustomInvokedCount++; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good optimization to have this, but I don't think it's strictly required. For "legacy" Activity flows we pass them through an ActivitySource for sampling. So the logic below should also catch these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodeBlanch Hmm 🤔, maybe I need you to point me to some code... because without this check here, this test I wrote would fail:
https://github.com/open-telemetry/opentelemetry-dotnet/pull/1079/files#diff-a266e12581b9c9ae7bd148bbf91dabb8R37-R59
So, it would seem that for legacy activities, we'll need a check somewhere in addition to the check I have in the ActivityListener for the new activities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, my bad.That was a test and you passed, great job!It looks like ActivitySourceAdapter has its own sampler, so we'll need to do similar in here:
opentelemetry-dotnet/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs
Lines 85 to 131 in 44b55d4
Or maybe we can refactor so more is shared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! Phew!
I don't think so because this is actually downstream of the check I have in place.
So,
DiagnosticSourceListener.OnNext
--- invokes --->ListenerHandler.On[Start|End|Exception|Custom]
It is our
ListenerHandler
s that hold an instance of theActivitySourceAdapter
which is then the thing that simulates Start/Stop/etc and deals with sampling like the standard ActivitySource/ActivityListener.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying it wouldn't work, or just that this spot is sooner? If the later, I suppose that is compelling enough a reason to keep it here and I'm good with it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would work for preventing
this.activityProcessor.OnStart(activity);
, but the reason I put the check where I did inDiagnosticSourceListener.OnNext
is that the check there prevents all instrumentation callbacks defined on theListenerHandler
from being invoked. For example, if some library had:If we had a
ListenerHander
wired up to this library then thisWrite
would result in invoking theOnCustom
callback. I figured it would be good to suppress all aspects of the library's instrumentation.Though I guess in most (if not all) of these callbacks we check
activity.IsAllDataRequested
, so they'd result in noops anyways...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense good call!