-
Notifications
You must be signed in to change notification settings - Fork 752
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
Added sampler to Activity - v1 #683
Merged
cijothomas
merged 10 commits into
open-telemetry:master
from
cijothomas:cijothomas/activity2sampler
May 21, 2020
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c44d367
Added sampler to Activity
cijothomas a3552b0
Merge branch 'master' into cijothomas/activity2sampler
cijothomas a6b03d0
modify comment
cijothomas 6407145
fix comments from PR feedbacks
cijothomas c93cb01
tests added
cijothomas c7c3205
Merge branch 'master' into cijothomas/activity2sampler
cijothomas 8f17e1b
Merge branch 'master' into cijothomas/activity2sampler
cijothomas 031e673
address pr comment
cijothomas 6bcca6c
Merge branch 'master' into cijothomas/activity2sampler
cijothomas 70b96d3
Merge branch 'master' into cijothomas/activity2sampler
cijothomas 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// <copyright file="ActivitySampler.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.Collections.Generic; | ||
using System.Diagnostics; | ||
|
||
namespace OpenTelemetry.Trace | ||
{ | ||
/// <summary> | ||
/// Sampler to select data to be exported. This sampler executes before Activity object is created. | ||
/// </summary> | ||
public abstract class ActivitySampler | ||
{ | ||
/// <summary> | ||
/// Gets the sampler description. | ||
/// </summary> | ||
public abstract string Description { get; } | ||
|
||
/// <summary> | ||
/// Checks whether activity needs to be created and tracked. | ||
/// </summary> | ||
/// <param name="parentContext">Parent activity context. Typically taken from the wire.</param> | ||
/// <param name="traceId">Trace ID of a activity to be created.</param> | ||
/// <param name="spanId">Span ID of a activity to be created.</param> | ||
/// <param name="name"> Name (DisplayName) of the activity to be created. Note, that the name of the activity is settable. | ||
/// So this name can be changed later and Sampler implementation should assume that. | ||
/// Typical example of a name change is when <see cref="Activity"/> representing incoming http request | ||
/// has a name of url path and then being updated with route name when routing complete. | ||
/// </param> | ||
/// <param name="activityKind">The kind of the Activity.</param> | ||
/// <param name="tags">Initial set of Tags for the Activity being constructed.</param> | ||
/// <param name="links">Links associated with the activity.</param> | ||
/// <returns>Sampling decision on whether activity needs to be sampled or not.</returns> | ||
public abstract SamplingResult ShouldSample(in ActivityContext parentContext, in ActivityTraceId traceId, in ActivitySpanId spanId, string name, ActivityKind activityKind, IEnumerable<KeyValuePair<string, string>> tags, IEnumerable<ActivityLink> links); | ||
cijothomas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
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
35 changes: 35 additions & 0 deletions
35
src/OpenTelemetry/Trace/Samplers/AlwaysOffActivitySampler.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,35 @@ | ||
// <copyright file="AlwaysOffActivitySampler.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.Collections.Generic; | ||
using System.Diagnostics; | ||
|
||
namespace OpenTelemetry.Trace.Samplers | ||
{ | ||
/// <summary> | ||
/// Sampler implementation which never samples any activity. | ||
/// </summary> | ||
public sealed class AlwaysOffActivitySampler : ActivitySampler | ||
{ | ||
/// <inheritdoc /> | ||
public override string Description { get; } = nameof(AlwaysOffActivitySampler); | ||
|
||
/// <inheritdoc /> | ||
public override SamplingResult ShouldSample(in ActivityContext parentContext, in ActivityTraceId traceId, in ActivitySpanId spanId, string name, ActivityKind activityKind, IEnumerable<KeyValuePair<string, string>> tags, IEnumerable<ActivityLink> links) | ||
{ | ||
return new SamplingResult(false); | ||
} | ||
} | ||
} |
36 changes: 36 additions & 0 deletions
36
src/OpenTelemetry/Trace/Samplers/AlwaysOnActivitySampler.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,36 @@ | ||
// <copyright file="AlwaysOnActivitySampler.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.Collections.Generic; | ||
using System.Diagnostics; | ||
|
||
namespace OpenTelemetry.Trace.Samplers | ||
{ | ||
/// <summary> | ||
/// Sampler implementation which samples every activity. | ||
/// This sampler will be used as the default Sampler, if no other Sampler is configured. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this comment should be on the builder to increase visibility. |
||
/// </summary> | ||
public sealed class AlwaysOnActivitySampler : ActivitySampler | ||
{ | ||
/// <inheritdoc /> | ||
public override string Description { get; } = nameof(AlwaysOnActivitySampler); | ||
|
||
/// <inheritdoc /> | ||
public override SamplingResult ShouldSample(in ActivityContext parentContext, in ActivityTraceId traceId, in ActivitySpanId spanId, string name, ActivityKind activityKind, IEnumerable<KeyValuePair<string, string>> tags, IEnumerable<ActivityLink> links) | ||
{ | ||
return new SamplingResult(true); | ||
} | ||
} | ||
} |
92 changes: 92 additions & 0 deletions
92
test/OpenTelemetry.Tests/Implementation/Trace/Samplers/ActivitySamplersTest.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,92 @@ | ||
// <copyright file="ActivitySamplersTest.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.Collections.Generic; | ||
using System.Diagnostics; | ||
using Xunit; | ||
|
||
namespace OpenTelemetry.Trace.Samplers.Test | ||
{ | ||
public class ActivitySamplersTest | ||
{ | ||
private static readonly ActivityKind ActivityKindServer = ActivityKind.Server; | ||
private readonly ActivityTraceId traceId; | ||
private readonly ActivitySpanId spanId; | ||
private readonly ActivitySpanId parentSpanId; | ||
|
||
public ActivitySamplersTest() | ||
{ | ||
traceId = ActivityTraceId.CreateRandom(); | ||
spanId = ActivitySpanId.CreateRandom(); | ||
parentSpanId = ActivitySpanId.CreateRandom(); | ||
} | ||
|
||
[Theory] | ||
[InlineData(ActivityTraceFlags.Recorded)] | ||
[InlineData(ActivityTraceFlags.None)] | ||
public void AlwaysOnSampler_AlwaysReturnTrue(ActivityTraceFlags flags) | ||
{ | ||
var parentContext = new ActivityContext(traceId, parentSpanId, flags); | ||
var link = new ActivityLink(parentContext); | ||
|
||
Assert.True( | ||
new AlwaysOnActivitySampler() | ||
.ShouldSample( | ||
parentContext, | ||
traceId, | ||
spanId, | ||
"Another name", | ||
ActivityKindServer, | ||
null, | ||
new List<ActivityLink>() { link }).IsSampled); | ||
} | ||
|
||
[Fact] | ||
public void AlwaysOnSampler_GetDescription() | ||
{ | ||
// TODO: The name must be AlwaysOnSampler as per spec. | ||
// We should correct it when we replace span sampler with this. | ||
Assert.Equal("AlwaysOnActivitySampler", new AlwaysOnActivitySampler().Description); | ||
} | ||
|
||
[Theory] | ||
[InlineData(ActivityTraceFlags.Recorded)] | ||
[InlineData(ActivityTraceFlags.None)] | ||
public void AlwaysOffSampler_AlwaysReturnFalse(ActivityTraceFlags flags) | ||
{ | ||
var parentContext = new ActivityContext(traceId, parentSpanId, flags); | ||
var link = new ActivityLink(parentContext); | ||
|
||
Assert.False( | ||
new AlwaysOffActivitySampler() | ||
.ShouldSample( | ||
parentContext, | ||
traceId, | ||
spanId, | ||
"Another name", | ||
ActivityKindServer, | ||
null, | ||
new List<ActivityLink>() { link }).IsSampled); | ||
} | ||
|
||
[Fact] | ||
public void AlwaysOffSampler_GetDescription() | ||
{ | ||
// TODO: The name must be AlwaysOffSampler as per spec. | ||
// We should correct it when we replace span sampler with this. | ||
Assert.Equal("AlwaysOffActivitySampler", new AlwaysOffActivitySampler().Description); | ||
} | ||
} | ||
} |
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.
I wonder whats the value of spanid here - the span/activity is not yet created, and will be done based on the outcome of this method.
Can't think of the use of spanid in a typical sampling algorithm, as new spanid is just random.
(With activity there is no option to know the spanid ahead of Activity creation, and no option to change spanid after creation).
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.
IIRC an OTel SDK should be able to generate the IDs before calling the sampler, and historically some distributed tracing systems actually used the span ID instead of the trace ID to make their decision.
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.
If this is a requirement, we need to address this,
Adding it here for tracking: #675 (comment)
cc: @tarekgh
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.
I am wondering how a SpanId (for a span which not created yet) will be used in the sampling decision? this looks weird to me as this Id never been used till the sampling time. do we expect the samplers recognize specific patterns of Span Ids to make the decision accordingly? I can understand it could make sense to use the Span Id for sampling if the span is already created but not if the span really not created at all.
@cijothomas maybe the trace id is interesting too here.
@noahfalk
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.
since traceid for parent, and the child-to-be-created are going to be the same, I dont see how traceid is an issue?
Unless parent is null?
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.
@reyang here and there I tried to dig something about this and I didn't see any PR/discussion mentioning the
SpanID
perhaps it was an oversight? Unless @SergeyKanzhelev has something to add we should propose a simple change of removing it to see if anyone complains :)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.
So far my archeology work traced back to OpenCensus 😆
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.
yeah 🤠 (they didn't have an Indiana Jones emoji but the cowboy has a hat 🤷♂️), it seems very much like OpenCensus - that said everything that I see is using traceId to make their decisions, now will be the moment to remove it before some "creative person" starts to depend on 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.
I've added this topic to Specification SIG meeting 05/26/2020 agenda.
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.
Discussed during the SIG meeting and agreed to remove it from the spec.
open-telemetry/opentelemetry-specification#621