[Exporter.Zipkin] Update remote endpoint and remove the peer.service attribute #6191

ysolomchenko · 2025-03-17T10:15:19Z

Changes

Removed the peer service resolver logic.
Extended remote endpoint calculation to align with the opentelemetry-specification.

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
~~Changes in public API reviewed (if applicable)~~

codecov · 2025-03-17T10:19:27Z

Codecov Report

Attention: Patch coverage is 96.87500% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.56%. Comparing base (31fb3bd) to head (e5fcf21).

Files with missing lines	Patch %	Lines
...plementation/ZipkinActivityConversionExtensions.cs	96.87%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6191      +/-   ##
==========================================
+ Coverage   86.43%   86.56%   +0.12%     
==========================================
  Files         258      258              
  Lines       11792    11834      +42     
==========================================
+ Hits        10193    10244      +51     
+ Misses       1599     1590       -9

Flag	Coverage Δ
unittests-Project-Experimental	`86.45% <96.87%> (+0.32%)`	⬆️
unittests-Project-Stable	`86.44% <96.87%> (+0.08%)`	⬆️
unittests-Solution	`86.21% <96.87%> (-0.06%)`	⬇️
unittests-UnstableCoreLibraries-Experimental	`85.78% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...plementation/ZipkinActivityConversionExtensions.cs	`95.62% <96.87%> (-0.14%)`	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj

...OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionExtensionsTest.cs

based on OTLP similar changes

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionTests.cs

...nTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityExporterRemoteEndpointTests.cs

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

ysolomchenko · 2025-03-24T10:29:13Z

Benchmark comparing new method CreateTestActivity and old method CreateTestActivity

benchmark:

CreateTestActivity taken from ZipkinActivitySource.cs

[MemoryDiagnoser]
[SimpleJob(RuntimeMoniker.Net80)]
[SimpleJob(RuntimeMoniker.Net90)]
public class ZipkinBenchmark
{
    private static readonly ZipkinEndpoint DefaultZipkinEndpoint = new("TestService");

    public static IEnumerable<KeyValuePair<string, Dictionary<string, object>>> NewMethodTestCases =>
    [
        new("peer.service provided", new Dictionary<string, object> { ["peer.service"] = "PeerService" }),
        new("No attributes", new Dictionary<string, object> { }),
    ];

    public static IEnumerable<KeyValuePair<string, Dictionary<string, object>>> OldMethodTestCases =>
    [
        new("net.peer.name provided", new Dictionary<string, object> { ["net.peer.name"] = "RemoteServiceName" }),
        new("No attributes", new Dictionary<string, object> { }),
    ];

    [Benchmark]
    [ArgumentsSource(nameof(NewMethodTestCases))]
    [BenchmarkCategory("NewMethod")]
    public void NewMethod(KeyValuePair<string, Dictionary<string, object>> testCase)
    {
        var (name, additionalAttributes) = testCase;
        var activity = ZipkinActivitySource.CreateTestActivity(additionalAttributes: additionalAttributes);
        ZipkinActivityConversionExtensionsNew.ToZipkinSpan(activity, DefaultZipkinEndpoint);
    }

    [Benchmark]
    [ArgumentsSource(nameof(OldMethodTestCases))]
    [BenchmarkCategory("OldMethod")]
    public void OldMethod(KeyValuePair<string, Dictionary<string, object>> testCase)
    {
        var (name, additionalAttributes) = testCase;
        var activity = ZipkinActivitySource.CreateTestActivity(additionalAttributes: additionalAttributes);
        ZipkinActivityConversionExtensionsOld.ToZipkinSpan(activity, DefaultZipkinEndpoint);
    }
}

Results (testCase column was modified for better readability):

The NewMethod is faster in the happy path, when you provide Highest Rank of Attribute "peer.service".
The NewMethod is slower in the unhappy path, when attributes not provided.

Method	Job	Runtime	testCase	Mean	Error	StdDev	Gen0	Gen1	Allocated
NewMethod	.NET 8.0	.NET 8.0	No attributes	1.861 us	0.0371 us	0.0985 us	0.3242	-	5.96 KB
NewMethod	.NET 9.0	.NET 9.0	No attributes	1.596 us	0.0311 us	0.0291 us	0.3319	0.0019	6.13 KB
OldMethod	.NET 8.0	.NET 8.0	No attributes	1.617 us	0.0323 us	0.0317 us	0.3242	0.0019	5.96 KB
OldMethod	.NET 9.0	.NET 9.0	No attributes	1.496 us	0.0296 us	0.0290 us	0.3319	0.0019	6.13 KB
NewMethod	.NET 8.0	.NET 8.0	peer.service provided	1.638 us	0.0325 us	0.0578 us	0.3262	0.0019	6 KB
NewMethod	.NET 9.0	.NET 9.0	peer.service provided	1.504 us	0.0294 us	0.0339 us	0.3338	0.0019	6.16 KB
OldMethod	.NET 8.0	.NET 8.0	net.peer.name provided	1.750 us	0.0349 us	0.0815 us	0.3262	0.0019	6 KB
OldMethod	.NET 9.0	.NET 9.0	net.peer.name provided	1.788 us	0.0643 us	0.1897 us	0.3338	0.0019	6.16 KB

Kielek · 2025-04-03T04:57:19Z

@TimothyMothra, @rajkumar-rangaraj, could you please review?

xiang17 · 2025-04-15T01:45:04Z

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

+
+    private static PooledList<ZipkinAnnotation> ExtractActivityEvents(Activity activity)
+    {
+        var annotations = PooledList<ZipkinAnnotation>.Create();


There is a slight difference here: if the activity.EnumerateEvents() is empty, the original code wouldn't create a PooledList because the initialization (this.Annotations = PooledList<ZipkinAnnotation>.Create();) in original code (link) is inside an if statement enumerator.MoveNext().

fixed in bf03c84

xiang17 · 2025-04-15T02:11:21Z

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

-
-        // In the case when both activity status and status tag were set,
-        // activity status takes precedence over status tag.
-        else if (tagState.StatusCode.HasValue && tagState.StatusCode != StatusCode.Unset)


Is the logic regarding tagState.StatusCode removed? Was there any update on the rule in comment Error flag rule from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/zipkin.md#status?

The logic for this has been moved to ExtractActivityStatus. I'm using a different approach: I'm not adding the error and status tags in ExtractActivityTags, and instead handling this in ExtractActivityStatus.

The old logic:

checks both activity status and status tag. Activity status takes precedence over status tag if both exist. Use status tag for StatusCode and StatusDescription if activity status is missing.

My question is the new logic never used StatusDescription from status tag like the old logic.

I realized this activity.GetTagItem is looking up in a LinkedList, so it does linear look up with O(n) time complexity in the new logic. Then it'll eliminate performance benefits introduced by these new APIs: #6191 (comment).

This is likely why the existing approach would enumerate the tags and save the results into tagState. This way the loop only happens once and we get all needed tag values (both StatusCode and StatusDescription) in one pass.

If I understand your question about StatusDescription (error tag) correctly: the specification states that StatusDescription should not be set for OK and UNSET, and that part is accounted for. I'm also not trying to set StatusCode to error just because StatusDescription is present, since StatusCode has higher priority.

If you're asking whether, in the case where Activity.Status is UNSET and StatusCode is null, and only a StatusDescription is present - should we then set Activity.Status and StatusCode to error based on that?

I think if Activity.Status is UNSET and StatusCode is null, we can treat the status as UNSET, ignoring the fact that StatusDescription is present.

The specification doesn't mention this particular case.

test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/RemoteEndpointPriorityTestCase.cs

xiang17 · 2025-04-15T02:48:07Z

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

+            return endpoint;
+        }
+
+        remoteEndpoint = activity.GetTagItem(SemanticConventions.AttributeHttpHost) as string;


The linked spec doesn't have this one. Is it kept for backward compatibility? If that's the case, maybe also keep a unit test case for this.

Yes, this is kept for backward compatibility. Test added in bf03c84

xiang17 · 2025-04-16T00:17:14Z

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

        {
-            foreach (ref readonly var tag in activity.EnumerateTagObjects())
+            if (activity.GetTagItem(SpanAttributeConstants.StatusCodeKey) is string status)


Mentioned in #6191 (comment): activity.GetTagItem is a linear look up.

xiang17 · 2025-04-16T00:39:09Z

src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs

+            return null;
+        }
+
+        string? remoteEndpoint = activity.GetTagItem(SemanticConventions.AttributePeerService) as string;


See the other comment: #6191 (comment): activity.GetTagItem is a linear look up.

Considering this action happens on every activity, it might be worth it to

loop once like the old EnumerateTags to cache all the possible attributes for remoteEndpoint and pick the highest ranking one (complexity is $O(n + k)$ ), rather than

do a loop for every possible attribute for remoteEndpoint (complexity is $O(k \cdot n + k)$ ).

(Assume the number of all the possible attributes for remoteEndpoint is $k$, and an activity has $n$ tags.)

I cache all tags to avoid using Activity.GetTagItem, not only in remoteEndpoint but also in StatusCode in this commit:2a0dacb

…com/ysolomchenko/opentelemetry-dotnet into remove-peer-service-resolver-zipkin

Remove the peer service resolver logic

a76d9ff

github-actions bot added the pkg:OpenTelemetry.Exporter.Zipkin Issues related to OpenTelemetry.Exporter.Zipkin NuGet package label Mar 17, 2025

Changelog

2eadb10

ysolomchenko marked this pull request as ready for review March 17, 2025 10:40

ysolomchenko requested a review from a team as a code owner March 17, 2025 10:40

Kielek reviewed Mar 17, 2025

View reviewed changes

src/OpenTelemetry.Exporter.Zipkin/OpenTelemetry.Exporter.Zipkin.csproj Show resolved Hide resolved

Kielek reviewed Mar 17, 2025

View reviewed changes

...OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionExtensionsTest.cs Outdated Show resolved Hide resolved

ysolomchenko and others added 6 commits March 17, 2025 12:26

Delete PeerServiceResolver file

e502a32

Refactor ZipkinActivityConversionExtensionsTest

3239c95

Merge branch 'main' into remove-peer-service-resolver-zipkin

d9004d5

Extend CHANGELOG

32fa33a

based on OTLP similar changes

typo fix

caafe81

IDE0019

7781bf5

Kielek requested changes Mar 18, 2025

View reviewed changes

ysolomchenko added 3 commits March 19, 2025 11:09

fix requested changes

ed44926

Merge branch 'main' into remove-peer-service-resolver-zipkin

8334d69

fix issues after merge

46a37b1

Kielek changed the title ~~[Exporter.Zipkin] Remove the peer service resolver logic~~ [Exporter.Zipkin] Update remote endpoitn and remove the peer.service attribute Mar 20, 2025

Kielek changed the title ~~[Exporter.Zipkin] Update remote endpoitn and remove the peer.service attribute~~ [Exporter.Zipkin] Update remote endpoint and remove the peer.service attribute Mar 20, 2025

Merge branch 'main' into remove-peer-service-resolver-zipkin

9d9ae51

Kielek approved these changes Mar 24, 2025

View reviewed changes

Kielek added 3 commits March 26, 2025 11:06

Merge branch 'main' into remove-peer-service-resolver-zipkin

cf9d218

Merge branch 'main' into remove-peer-service-resolver-zipkin

9b56972

Merge branch 'main' into remove-peer-service-resolver-zipkin

80ec794

Kielek added 3 commits April 3, 2025 06:57

Merge branch 'main' into remove-peer-service-resolver-zipkin

8bd03c7

Merge branch 'main' into remove-peer-service-resolver-zipkin

e9367eb

Merge branch 'main' into remove-peer-service-resolver-zipkin

9d78dc5

Merge branch 'main' into remove-peer-service-resolver-zipkin

666b535

xiang17 reviewed Apr 15, 2025

View reviewed changes

ysolomchenko and others added 2 commits April 15, 2025 11:20

resolve suggestions

bf03c84

Merge branch 'main' into remove-peer-service-resolver-zipkin

2bd5550

xiang17 reviewed Apr 16, 2025

View reviewed changes

ysolomchenko added 3 commits April 16, 2025 13:01

Create cached tags to avoid using Activity.GetTagItem

2a0dacb

Merge branch 'remove-peer-service-resolver-zipkin' of https://github.…

a73d21e

…com/ysolomchenko/opentelemetry-dotnet into remove-peer-service-resolver-zipkin

fix

e5fcf21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Exporter.Zipkin] Update remote endpoint and remove the peer.service attribute #6191

[Exporter.Zipkin] Update remote endpoint and remove the peer.service attribute #6191

ysolomchenko commented Mar 17, 2025 •

edited

Loading

codecov bot commented Mar 17, 2025 •

edited

Loading

ysolomchenko commented Mar 24, 2025 •

edited

Loading

Kielek commented Apr 3, 2025

xiang17 Apr 15, 2025

ysolomchenko Apr 15, 2025

xiang17 Apr 15, 2025

ysolomchenko Apr 15, 2025

xiang17 Apr 15, 2025 •

edited

Loading

xiang17 Apr 16, 2025 •

edited

Loading

ysolomchenko Apr 16, 2025

xiang17 Apr 15, 2025

ysolomchenko Apr 15, 2025

xiang17 Apr 16, 2025

xiang17 Apr 16, 2025

ysolomchenko Apr 16, 2025

[Exporter.Zipkin] Update remote endpoint and remove the peer.service attribute #6191

Are you sure you want to change the base?

[Exporter.Zipkin] Update remote endpoint and remove the peer.service attribute #6191

Conversation

ysolomchenko commented Mar 17, 2025 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Mar 17, 2025 • edited Loading

Codecov Report

ysolomchenko commented Mar 24, 2025 • edited Loading

Kielek commented Apr 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang17 Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

xiang17 Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysolomchenko commented Mar 17, 2025 •

edited

Loading

codecov bot commented Mar 17, 2025 •

edited

Loading

ysolomchenko commented Mar 24, 2025 •

edited

Loading

xiang17 Apr 15, 2025 •

edited

Loading

xiang17 Apr 16, 2025 •

edited

Loading