Include net.peer.port in ZipkinSpan.RemoteEndpoint.ServiceName #1168
Conversation
test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1168 +/- ##
==========================================
+ Coverage 77.44% 77.46% +0.02%
==========================================
Files 219 219
Lines 6255 6270 +15
==========================================
+ Hits 4844 4857 +13
- Misses 1411 1413 +2
|
| @@ -177,6 +177,7 @@ public void ZipkinExporterIntegrationTest(bool useShortTraceIds) | |||
| internal static Activity CreateTestActivity( | |||
| bool setAttributes = true, | |||
| Dictionary<string, object> additionalAttributes = null, | |||
| bool convertAttributeValuesToString = true, | |||
There was a problem hiding this comment.
I introduced this to enable testing the code path in ProcessTags that handles things when net.peer.port is an integer. Meh? Better ideas?
The net452 compilation uses Newtonsoft.Json and other compilations use the newer System.Text.Json.
My understanding is that this helper method does a ToString on all attribute values to eliminate any serialization differences between net452 and other target compilations. That is, there are some tests (not touched by this PR) that depend on the result of serialization being the same. Namely here:
Should we be at all concerned that these two compilations result in serializing the Zipkin payload differently?
There was a problem hiding this comment.
Zipkin will send numeric attribute values for the net452 compilation that uses Newtonsoft.Json. If you remove the ToString() from here and run the Zipkin tests, you'll see what I mean:
So I think this ToString is masking the bug because it means that the Activity created from this helper method will only ever have string attributes, so I don't think we are testing the serialization of the Zipkin payload when an Activity has non-string attributes.
I simply needed to be able to test the code path I put in place that does special handling of the net.peer.port attribute when it is an int, and by removing the ToString() it seems I uncovered the bug.
If the Zipkin spec states that everything should be sent as strings then I think we should remove this ToString and fix the bug. If we do that, then the convertAttributeValuesToString parameter I've introduced here would be unneeded.
There was a problem hiding this comment.
@alanwest, I think we should change to bet 452 as well. When I changed the others to output as string I didn't look at net452. Let me know if u will change or I can open another pr
There was a problem hiding this comment.
@eddynaka if you're interested in doing that PR, that would be super. I think what I will do for now is revert the changes to this helper method, but I think that if you land a fix for 452 then this ToString() should be able to be removed.
There was a problem hiding this comment.
just opened a pr solving this!
| if ((attributeEnumerationState.RemoteEndpointServiceName == null || attributeEnumerationState.RemoteEndpointServiceNamePriority > 0) | ||
| && hostNameOrIpAddress != null) | ||
| { | ||
| var remoteEndpointStr = attributeEnumerationState.Port != default |
There was a problem hiding this comment.
@CodeBlanch I'm not opposed to using a caching mechanism here to avoid string allocations, but I want to be sure to avoid any risk of unbounded memory growth of the cache. I could make a cache local to an individual export operation, but that might limit any perf gain.
It's not immediately apparent to me if the StackExchangeRedisCallsInstrumentation avoids unbounded memory growth. What, if anything, ever clears items from the cache?
There was a problem hiding this comment.
@alanwest The one in Redis should only be caching live traces/spans. They should remove as they complete. This cache here, will grow unbounded. It is cheating/assuming that the user will only ever connect to a reasonable set of remote services. That assumption may or may not be accurate, and we might want to add some expiration? But moving from a string key to a ValueTuple doesn't really change that 😄
There was a problem hiding this comment.
The one in Redis should only be caching live traces/spans. They should remove as they complete.
Hmm... not seeing where this happens - I'm not very familiar with the Redis instrumentation. Though, I will parking-lot this concern since it's not related to this PR.
It is cheating/assuming that the user will only ever connect to a reasonable set of remote services.
I think it's safer to not make this assumption. I think it is often the case that the set remote services is of a reasonable size, but I've seen numerous instances in customers applications where this set can be very large. This can be common in mutli-tenant architectures where number of hostnames can be large and hostname is used to route to a specific tenant. I think it might make sense to come back to the thought of a cache expiration in a separate issue.
But moving from a string key to a ValueTuple doesn't really change that
Agreed, I've made this change since it seems like a solid approach. Note that the net452 build still uses a string as a key. Could have made this a regular Tuple, but I figured trading a string allocation for a Tuple allocation for net452 didn't make a lot of sense.
This reverts commit c88120d.
Fixes #968.
@CodeBlanch I'm particularly interested in your review of this since I think you've worked in the space more than most. I'm not terribly excited about how things turned out, but at least should provide a starting place for discussion.
Once we settle on this PR, I will follow up with a similar PR for Jaeger.
Changes
If
net.peer.nameornet.peer.ipattribute is present thennet.peer.portshould be included (if present) when setting the remote service endpoint for a Zipkin span.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes