Add ActivityExporter for Zipkin #738
Conversation
src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
Outdated
Show resolved
Hide resolved
| return timestamp.ToUnixTimeMilliseconds() * 1000; | ||
| } | ||
|
|
||
| internal static long ToEpochMicroseconds(this DateTime utcDateTime) |
There was a problem hiding this comment.
@cijothomas @CodeBlanch I think this might be a good function to move to a shared file (and use links to share across multiple projects)?
src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
Outdated
Show resolved
Hide resolved
| state.RemoteEndpointServiceName = strVal; | ||
| state.RemoteEndpointServiceNamePriority = priority; | ||
| } | ||
| else if (key == Resource.ServiceNameKey) |
There was a problem hiding this comment.
Curious, wonder if "if-else" or a dict/hash look up would be more performant. Probably won't matter as it is not on the hot path.
| { | ||
| try | ||
| { | ||
| await this.SendBatchActivityAsync(batchActivity).ConfigureAwait(false); |
There was a problem hiding this comment.
This part needs to be revisited since it doesn't seem to be deterministic. Not a blocker for this PR though.
reyang
left a comment
reyang
left a comment
There was a problem hiding this comment.
Looks good in general. The time/duration precision part seems to be in question. I've also left some non-blocking comments. I approve assuming the precision issue will be fixed.
CodeBlanch
left a comment
CodeBlanch
left a comment
There was a problem hiding this comment.
@rajkumar-rangaraj If you have the bandwidth it might be useful to add ZipActivityExporter_ExportAsync benchmark next to...
...so we can run a quick comparison of the two for perf regressions?
src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionTest.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinActivityExporterTests.cs
Outdated
Show resolved
Hide resolved
4 participants
Fixes #721 .
Changes
Add ActivityExporter for Zipkin. Used same patterns already in the Zipkin Exporter and enabled exporter to transform Activity into Zipkin spans. Used CreateTestActivity as in JaegerActivity Exporter.
Checklist