Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,24 @@ public static object GetTagValue(this Activity activity, string tagName)
return null;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool TryGetTagValue(this Activity activity, string tagName, out object tagValue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How strongly do you feel about using the existing method?

I think the TryGet is cleaner and saves an unnecessary assertion (if returnValue != null).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the min bar is:

  1. don't duplicate code (e.g. if we want to add this method, at least the existing one GetTagValue should be updated so it'll leverage the TryGetTagValue).
  2. keep it consistent - examine the existing usage and make proper update.

Copy link
Contributor

@utpilla utpilla Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saves an unnecessary assertion if (returnValue != null)

Even with the new method you have to check if (returnValue == true).

I would be in favor of reusing the existing method in this case and avoid having to introduce more code (and related tests) just to achieve a boolean check instead of a null check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of reusing the existing method in this case and avoid having to introduce more code (and related tests) just to achieve a boolean check instead of a null check.

+1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

{
Debug.Assert(activity != null, "Activity should not be null");

foreach (ref readonly var tag in activity.EnumerateTagObjects())
{
if (tag.Key == tagName)
{
tagValue = tag.Value;
return true;
}
}

tagValue = null;
return false;
}

/// <summary>
/// Checks if the user provided tag name is the first tag of the <see cref="Activity"/> and retrieves the tag value.
/// </summary>
Expand Down
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
which attributes are emitted by setting the environment variable
`OTEL_SEMCONV_STABILITY_OPT_IN`.

* Fixed an issue affecting NET 7.0+. If custom propagation is being used
and tags are added to an Activity during sampling then that Activity would be dropped.
([#4637](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4637))

## 1.5.0-beta.1

Released 2023-Jun-05
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,11 @@ public void OnStopActivity(Activity activity, object payload)
}
}

#if NET7_0_OR_GREATER
if (activity.TryGetTagValue("IsCreatedByInstrumentation", out var tagValue) && ReferenceEquals(tagValue, bool.TrueString))
#else
if (activity.TryCheckFirstTag("IsCreatedByInstrumentation", out var tagValue) && ReferenceEquals(tagValue, bool.TrueString))
#endif
{
// If instrumentation started a new Activity, it must
// be stopped here.
Expand Down
24 changes: 24 additions & 0 deletions test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,30 @@ public void GetTagValue()
Assert.Null(activity.GetTagValue("Tag2"));
}

[Fact]
public void TryGetTagValue_Empty()
{
using var activity = new Activity("Test");

Assert.False(activity.TryGetTagValue("Tag1", out var value));
Assert.Null(value);
}

[Fact]
public void TryGetTagValue()
{
using var activity = new Activity("Test");
activity.SetTag("Tag1", "Value1");

Assert.True(activity.TryGetTagValue("Tag1", out var value));
Assert.Equal("Value1", value);

Assert.False(activity.TryGetTagValue("tag1", out value));
Assert.Null(value);
Assert.False(activity.TryGetTagValue("Tag2", out value));
Assert.Null(value);
}

[Theory]
[InlineData("Key", "Value", true)]
[InlineData("CustomTag", null, false)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,11 @@ public async Task CustomPropagator()
builder.ConfigureTestServices(services =>
{
Sdk.SetDefaultTextMapPropagator(propagator.Object);
this.tracerProvider = Sdk.CreateTracerProviderBuilder().AddAspNetCoreInstrumentation().AddInMemoryExporter(exportedItems).Build();
this.tracerProvider = Sdk.CreateTracerProviderBuilder()
.SetSampler(new TestSampler(SamplingDecision.RecordAndSample, new Dictionary<string, object> { { "SomeTag", "SomeKey" }, }))
.AddAspNetCoreInstrumentation()
.AddInMemoryExporter(exportedItems)
.Build();
});
builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders());
}))
Expand Down Expand Up @@ -1134,15 +1138,17 @@ public override void Inject<T>(PropagationContext context, T carrier, Action<T,
private class TestSampler : Sampler
{
private readonly SamplingDecision samplingDecision;
private readonly IEnumerable<KeyValuePair<string, object>> attributes;

public TestSampler(SamplingDecision samplingDecision)
public TestSampler(SamplingDecision samplingDecision, IEnumerable<KeyValuePair<string, object>> attributes = null)
{
this.samplingDecision = samplingDecision;
this.attributes = attributes;
}

public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
{
return new SamplingResult(this.samplingDecision);
return new SamplingResult(this.samplingDecision, this.attributes);
}
}

Expand Down