[shared] Add TagWriter implementation #5476
Conversation
|
I took a glance at this PR. The overall structure is good. However, it is very difficult to review it in this format. Could you please add some examples of consumption in the PR description or tests? |
|
@rajkumar-rangaraj Here are a few implementations of Zipkin case is the best. It writes directly onto the stream. That is ideally what we would do in OtlpExporter. |
| threadWriter = new Utf8JsonWriter(threadStream); | ||
| return new(threadStream, threadWriter); | ||
| } | ||
| else |
There was a problem hiding this comment.
When is this else clause executed, if the tags collection contains more than one array type? I'm trying to understand the scope of a thread and this struct allocation on the stack.
There was a problem hiding this comment.
The first time something is serialized with a tag value containing an array we create the storage. We use [ThreadStatic] so it will happen per thread. For exporters using the batch export processor there is a dedicated thread so that will happen once. For exporters using simple or something like reentrant, each thread can potentially have the storage. The else clause happens for all the calls after the first one. For that case we reset the state of storage so that it can be reused.
| case sbyte[] sbyteArray: this.WriteToArray(arrayState, sbyteArray); break; | ||
| case short[] shortArray: this.WriteToArray(arrayState, shortArray); break; | ||
| case ushort[] ushortArray: this.WriteToArray(arrayState, ushortArray); break; | ||
| #if NETFRAMEWORK |
There was a problem hiding this comment.
Instead of repeating #if NETFRAMEWORK we could get int[], unint[], long[] under one block.
There was a problem hiding this comment.
@CodeBlanch - Was this change reverted?
There was a problem hiding this comment.
Woops yes sorry about that I just put it back.
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5476 +/- ##
==========================================
+ Coverage 83.38% 85.51% +2.12%
==========================================
Files 297 289 -8
Lines 12531 12583 +52
==========================================
+ Hits 10449 10760 +311
+ Misses 2082 1823 -259
Flags with carried forward coverage won't be shown. Click here to find out more. |
Relates to #5455
Relates to #5475
Changes
TagWriterimplementation which will be used to replaceTagTransformer. Nothing usesTagWriteras of this PR.Details
I spun up #5475 to track issues I found while working on #5455. What I want to do is replace
TagTransformercompletely withTagWriteras a precursor to improving the performance of the OtlpExporter (#5450). WhatTagWriterprimarily achieves is the ability to write directly onto something instead of returning something which must then be written.Merge requirement checklist