Cleaning Jaeger Exporter #832

eddynaka · 2020-07-16T01:52:16Z

Thanks @CodeBlanch for the help!

Changes

Cleaning Jaeger Exporter

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public API reviewed

codecov · 2020-07-16T01:58:39Z

Codecov Report

Merging #832 into master will decrease coverage by 1.23%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
- Coverage   70.16%   68.93%   -1.24%     
==========================================
  Files         227      226       -1     
  Lines        6959     6833     -126     
  Branches     1150     1131      -19     
==========================================
- Hits         4883     4710     -173     
- Misses       1773     1822      +49     
+ Partials      303      301       -2

Impacted Files	Coverage Δ
....Jaeger/Implementation/JaegerActivityExtensions.cs	`85.27% <30.00%> (-3.81%)`	⬇️
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs	`100.00% <100.00%> (ø)`
...emetry.Exporter.Jaeger/Implementation/JaegerTag.cs	`47.36% <0.00%> (-22.11%)`	⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`59.72% <0.00%> (-13.89%)`	⬇️
...r.Jaeger/ApacheThrift/Protocol/TCompactProtocol.cs	`28.24% <0.00%> (-5.37%)`	⬇️
src/OpenTelemetry.Api/Trace/Status.cs	`69.76% <0.00%> (-4.66%)`	⬇️
...penTelemetry/Trace/Export/BatchingSpanProcessor.cs	`21.05% <0.00%> (-2.64%)`	⬇️
src/OpenTelemetry/Trace/Internal/EvictingQueue.cs	`85.71% <0.00%> (-2.39%)`	⬇️
...elemetry/Trace/Export/BatchingActivityProcessor.cs	`75.58% <0.00%> (-2.33%)`	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	`90.90% <0.00%> (-1.52%)`	⬇️
... and 1 more

CodeBlanch · 2020-07-16T02:06:07Z

test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerUdpBatcherTests.cs

+                links,
+                startTime: startTimestamp);
+
+            typeof(Activity).GetField("_traceId", BindingFlags.Instance | BindingFlags.NonPublic).SetValue(activity, traceId.ToHexString());


@tarekgh @cijothomas @reyang Just following up on this comment.

Here's the unit test code I was working on.

Modifying Links is really hard after construction because it uses an internal type. I ended up going through ActivitySource.StartActivity just so I could pass them in. Also allowed me to specify ActivityKind.

I needed this test to be deterministic so I set traceId, spanId, & parentId through reflection. I don't think there is a way in the API to set them otherwise.

Providing just as FYI. This isn't real-world usage obviously.

parentId can be set using activity.SetParentId("00-0123456789abcdef0123456789abcdef-0123456789abcdef-01");

Why not pass parentid when calling activitySource.StartActivity?

traceId can be passed in via ActivityContext.

@cijothomas I don't know a public way of specifying spanId for an activity, and I believe it was intended.

Thats right. No way to set spanId. TraceId and ParentSpanId, can be set while starting.

@CodeBlanch @reyang @cijothomas , i tried to change to use the method SetParentId. When i do that, it seens it changes internally something that makes the test non-deterministic -> so, the bytearray gets different.

The code that i tried:
activity.SetParentId(parentSpanId.ToHexString());
activity.SetParentId(traceId, spanId, ActivityTraceFlags.None);
activity.SetParentId(traceId, spanId, ActivityTraceFlags.Recorded);

@eddynaka You still need to use reflection to set spanid.

@cijothomas i didn't remove the spanId reflection. I just removed the parentId reflection and tried to use the above calls.

@eddynaka confirmed reflection is working to make this predictable. We can follow up separately to see why setting parentid using the Setparentid is not working.

CodeBlanch · 2020-07-16T02:15:55Z

test/OpenTelemetry.Exporter.Jaeger.Tests/Implementation/JaegerUdpBatcherTests.cs

 using Xunit;

 namespace OpenTelemetry.Exporter.Jaeger.Tests.Implementation
 {
    public class JaegerUdpBatcherTests
    {
-        public const string TestPayloadBase64 = "goEBCWVtaXRCYXRjaBwcGAx0ZXN0IHByb2Nlc3MZHBgQdGVzdF9wcm9jZXNzX3RhZxUAGAp0ZXN0X3ZhbHVlAAAZHBab5cuG2OehhdwBFuPakI2n2cCVLhaAjfWp6NHt6dQBFrK5moSni5GXGBgETmFtZRkcFQAWm+XLhtjnoYXcARbj2pCNp9nAlS4W/Y6j+bqS9fbuAQAVAhaAgLPexpa/BRaAnJw5GYwYCXN0cmluZ0tleRUAGAV2YWx1ZQAYB2xvbmdLZXkVBkYCABgIbG9uZ0tleTIVBkYCABgJZG91YmxlS2V5FQInAAAAAAAA8D8AGApkb3VibGVLZXkyFQInAAAAAAAA8D8AGAdib29sS2V5FQQxABgJc3Bhbi5raW5kFQAYBmNsaWVudAAYDm90LnN0YXR1c19jb2RlFQAYAk9rABksFoCAs97Glr8FGSwYA2tleRUAGAV2YWx1ZQAYB21lc3NhZ2UVABgGRXZlbnQxAAAWgICz3saWvwUZLBgDa2V5FQAYBXZhbHVlABgHbWVzc2FnZRUAGAZFdmVudDIAAAAAAA==";
+        public const string TestPayloadBase64 = "goEBCWVtaXRCYXRjaBwcGAx0ZXN0IHByb2Nlc3MZHBgQdGVzdF9wcm9jZXNzX3RhZxUAGAp0ZXN0X3ZhbHVlAAAZHBab5cuG2OehhdwBFuPakI2n2cCVLhaAjfWp6NHt6dQBFrK5moSni5GXGBgETmFtZRkcFQAWm+XLhtjnoYXcARbj2pCNp9nAlS4W/Y6j+bqS9fbuAQAVABaAgLPexpa/BRYAGZwYCXN0cmluZ0tleRUAGAV2YWx1ZQAYB2xvbmdLZXkVABgBMQAYCGxvbmdLZXkyFQAYATEAGAlkb3VibGVLZXkVABgBMQAYCmRvdWJsZUtleTIVABgBMQAYB2Jvb2xLZXkVABgEVHJ1ZQAYDm90LnN0YXR1c19jb2RlFQAYAk9rABgJc3Bhbi5raW5kFQAYBmNsaWVudAAYDGxpYnJhcnkubmFtZRUAGBtDcmVhdGVUZXN0UGF5bG9hZEphZWdlclNwYW4AGSwWgICz3saWvwUZLBgDa2V5FQAYBXZhbHVlABgHbWVzc2FnZRUAGAZFdmVudDEAABaAgLPexpa/BRksGANrZXkVABgFdmFsdWUAGAdtZXNzYWdlFQAYBkV2ZW50MgAAAAAA";


This test is validating the thrift blob. A couple changes made moving to Activity (from Span) broke it. I got it as close as I could to what we had before and then re-captured a new blob.

Activity version:

0000 82 81 01 09 65 6D 69 74 42 61 74 63 68 1C 1C 18 ....emitBatch�� 0010 0C 74 65 73 74 20 70 72 6F 63 65 73 73 19 1C 18 .test process�� 0020 10 74 65 73 74 5F 70 72 6F 63 65 73 73 5F 74 61 .test_process_ta 0030 67 15 00 18 0A 74 65 73 74 5F 76 61 6C 75 65 00 g�.�.test_value. 0040 00 19 1C 16 9B E5 CB 86 D8 E7 A1 85 DC 01 16 E3 .��..........�. 0050 DA 90 8D A7 D9 C0 95 2E 16 80 8D F5 A9 E8 D1 ED ........�....... 0060 E9 D4 01 16 B2 B9 9A 84 A7 8B 91 97 18 18 04 4E ...�........��.N 0070 61 6D 65 19 1C 15 00 16 9B E5 CB 86 D8 E7 A1 85 ame��.�........ 0080 DC 01 16 E3 DA 90 8D A7 D9 C0 95 2E 16 FD 8E A3 ..�.........�... 0090 F9 BA 92 F5 F6 EE 01 00 15 00 16 80 80 B3 DE C6 ........�.�..... 00A0 96 BF 05 16 00 19 9C 18 09 73 74 72 69 6E 67 4B ...�.�.�.stringK 00B0 65 79 15 00 18 05 76 61 6C 75 65 00 18 07 6C 6F ey�.�.value.�.lo 00C0 6E 67 4B 65 79 15 00 18 01 31 00 18 08 6C 6F 6E ngKey�.�.1.�.lon 00D0 67 4B 65 79 32 15 00 18 01 31 00 18 09 64 6F 75 gKey2�.�.1.�.dou 00E0 62 6C 65 4B 65 79 15 00 18 01 31 00 18 0A 64 6F bleKey�.�.1.�.do 00F0 75 62 6C 65 4B 65 79 32 15 00 18 01 31 00 18 07 ubleKey2�.�.1.�. 0100 62 6F 6F 6C 4B 65 79 15 00 18 04 54 72 75 65 00 boolKey�.�.True. 0110 18 0E 6F 74 2E 73 74 61 74 75 73 5F 63 6F 64 65 �.ot.status_code 0120 15 00 18 02 4F 6B 00 18 09 73 70 61 6E 2E 6B 69 �.�.Ok.�.span.ki 0130 6E 64 15 00 18 06 63 6C 69 65 6E 74 00 18 0C 6C nd�.�.client.�.l 0140 69 62 72 61 72 79 2E 6E 61 6D 65 15 00 18 1B 43 ibrary.name�.��C 0150 72 65 61 74 65 54 65 73 74 50 61 79 6C 6F 61 64 reateTestPayload 0160 4A 61 65 67 65 72 53 70 61 6E 00 19 2C 16 80 80 JaegerSpan.�,�.. 0170 B3 DE C6 96 BF 05 19 2C 18 03 6B 65 79 15 00 18 ......�,�.key�.� 0180 05 76 61 6C 75 65 00 18 07 6D 65 73 73 61 67 65 .value.�.message 0190 15 00 18 06 45 76 65 6E 74 31 00 00 16 80 80 B3 �.�.Event1..�... 01A0 DE C6 96 BF 05 19 2C 18 03 6B 65 79 15 00 18 05 .....�,�.key�.�. 01B0 76 61 6C 75 65 00 18 07 6D 65 73 73 61 67 65 15 value.�.message� 01C0 00 18 06 45 76 65 6E 74 32 00 00 00 00 00 .�.Event2.....

Span version:

0000 82 81 01 09 65 6D 69 74 42 61 74 63 68 1C 1C 18 ....emitBatch�� 0010 0C 74 65 73 74 20 70 72 6F 63 65 73 73 19 1C 18 .test process�� 0020 10 74 65 73 74 5F 70 72 6F 63 65 73 73 5F 74 61 .test_process_ta 0030 67 15 00 18 0A 74 65 73 74 5F 76 61 6C 75 65 00 g�.�.test_value. 0040 00 19 1C 16 9B E5 CB 86 D8 E7 A1 85 DC 01 16 E3 .��..........�. 0050 DA 90 8D A7 D9 C0 95 2E 16 80 8D F5 A9 E8 D1 ED ........�....... 0060 E9 D4 01 16 B2 B9 9A 84 A7 8B 91 97 18 18 04 4E ...�........��.N 0070 61 6D 65 19 1C 15 00 16 9B E5 CB 86 D8 E7 A1 85 ame��.�........ 0080 DC 01 16 E3 DA 90 8D A7 D9 C0 95 2E 16 FD 8E A3 ..�.........�... 0090 F9 BA 92 F5 F6 EE 01 00 15 02 16 80 80 B3 DE C6 ........�.�..... 00A0 96 BF 05 16 80 9C 9C 39 19 8C 18 09 73 74 72 69 ...�...9�.�.stri 00B0 6E 67 4B 65 79 15 00 18 05 76 61 6C 75 65 00 18 ngKey�.�.value.� 00C0 07 6C 6F 6E 67 4B 65 79 15 06 46 02 00 18 08 6C .longKey�.F..�.l 00D0 6F 6E 67 4B 65 79 32 15 06 46 02 00 18 09 64 6F ongKey2�.F..�.do 00E0 75 62 6C 65 4B 65 79 15 02 27 00 00 00 00 00 00 ubleKey�.'...... 00F0 F0 3F 00 18 0A 64 6F 75 62 6C 65 4B 65 79 32 15 .?.�.doubleKey2� 0100 02 27 00 00 00 00 00 00 F0 3F 00 18 07 62 6F 6F .'.......?.�.boo 0110 6C 4B 65 79 15 04 31 00 18 09 73 70 61 6E 2E 6B lKey�.1.�.span.k 0120 69 6E 64 15 00 18 06 63 6C 69 65 6E 74 00 18 0E ind�.�.client.�. 0130 6F 74 2E 73 74 61 74 75 73 5F 63 6F 64 65 15 00 ot.status_code�. 0140 18 02 4F 6B 00 19 2C 16 80 80 B3 DE C6 96 BF 05 �.Ok.�,�........ 0150 19 2C 18 03 6B 65 79 15 00 18 05 76 61 6C 75 65 �,�.key�.�.value 0160 00 18 07 6D 65 73 73 61 67 65 15 00 18 06 45 76 .�.message�.�.Ev 0170 65 6E 74 31 00 00 16 80 80 B3 DE C6 96 BF 05 19 ent1..�........� 0180 2C 18 03 6B 65 79 15 00 18 05 76 61 6C 75 65 00 ,�.key�.�.value. 0190 18 07 6D 65 73 73 61 67 65 15 00 18 06 45 76 65 �.message�.�.Eve 01A0 6E 74 32 00 00 00 00 00 nt2.....

Known differences:

Activity pushes library.name from ActivitySource.Name. That wasn't in the span version but I left it in because that was an intentional addition (I hope).

Row 00A0 has some differences. I'm hoping that is due to new calculation of date which results in a slightly different number than before. That's a WAG.

Changes:

I had to fix up the SetStatus helper extension method so it didn't push null descriptions into tags. The behavior before was we only send description if there is some text.

test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterTests.cs

Updating tests removing JaegerConversionExtensions renaming methods Fixed up Jaeger unit tests and status code helper. removing unused references

CodeBlanch · 2020-07-18T03:48:46Z

src/OpenTelemetry.Api/Trace/ActivityExtensions.cs

@@ -41,7 +41,10 @@ public static void SetStatus(this Activity activity, Status status)
            }

            activity.AddTag(SpanAttributeConstants.StatusCodeKey, SpanHelper.GetCachedCanonicalCodeString(status.CanonicalCode));
-            activity.AddTag(SpanAttributeConstants.StatusDescriptionKey, status.Description);
+            if (!string.IsNullOrEmpty(status.Description))


eddynaka requested a review from a team July 16, 2020 01:52

CodeBlanch reviewed Jul 16, 2020

View reviewed changes

reyang reviewed Jul 16, 2020

View reviewed changes

test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterTests.cs Outdated Show resolved Hide resolved

cijothomas approved these changes Jul 17, 2020

View reviewed changes

Cleaning Jaeger Exporter

9a8d564

Updating tests removing JaegerConversionExtensions renaming methods Fixed up Jaeger unit tests and status code helper. removing unused references

cijothomas merged commit 595f8c2 into open-telemetry:master Jul 17, 2020

CodeBlanch reviewed Jul 18, 2020

View reviewed changes

CodeBlanch mentioned this pull request Jul 18, 2020

Updated to latest OpenTelemetry and ASP.NET Core requests are no longer recorded #785

Closed

eddynaka deleted the feature/cleaning-jaeger branch July 21, 2020 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleaning Jaeger Exporter #832

Cleaning Jaeger Exporter #832

eddynaka commented Jul 16, 2020 •

edited

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading

CodeBlanch Jul 16, 2020

reyang Jul 16, 2020

cijothomas Jul 16, 2020

reyang Jul 16, 2020

reyang Jul 16, 2020

cijothomas Jul 16, 2020

eddynaka Jul 16, 2020

cijothomas Jul 16, 2020

eddynaka Jul 16, 2020

cijothomas Jul 17, 2020

CodeBlanch Jul 16, 2020

CodeBlanch Jul 18, 2020

Cleaning Jaeger Exporter #832

Cleaning Jaeger Exporter #832

Conversation

eddynaka commented Jul 16, 2020 • edited Loading

Changes

codecov bot commented Jul 16, 2020 • edited Loading

Codecov Report

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

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

eddynaka commented Jul 16, 2020 •

edited

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading