-
Notifications
You must be signed in to change notification settings - Fork 772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[otlp] Switch to TagWriter for handling of tags/attributes #5585
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5585 +/- ##
==========================================
+ Coverage 83.38% 85.72% +2.33%
==========================================
Files 297 271 -26
Lines 12531 11369 -1162
==========================================
- Hits 10449 9746 -703
+ Misses 2082 1623 -459
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -80,7 +79,7 @@ internal abstract class TagWriter<TTagState, TArrayState> | |||
default: | |||
try | |||
{ | |||
var stringValue = TruncateString(Convert.ToString(tag.Value, CultureInfo.InvariantCulture), tagValueMaxLength); | |||
var stringValue = TruncateString(Convert.ToString(tag.Value/*TODO: , CultureInfo.InvariantCulture*/), tagValueMaxLength); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why is this changed to TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing TagTransformer<T>
we are migrating from doesn't use InvariantCulture
today:
var stringValue = TruncateString(Convert.ToString(tag.Value), tagValueMaxLength); |
There is a unit test in OtlpExporter which is adding a DateTime.UtcNow
as a tag value. With this change to use InvariantCulture
that test fails because the format changes.
I discussed a bit with @alanwest. We feel this is a bug. We should be using InvariantCulture
in the transformation code. But it is also a breaking change. So the idea was let's do this migration to TagWriter<T>
without changing that behavior. Then we can do a dedicated PR/fix for that after the migration so it is independent/clearly documented.
Follow-up to #5476
Relates to #5475
Changes
TagTransformer
toTagWriter
design in OtlpExporter projectDetails
See #5475 for information on why this is being done. The TL;DR version is this is set up to write directly onto a stream/buffer and skip all allocations completely (future work). Also there is more utilization of the base class/less duplication of logic in the tag handling for array values inside OtlpExporter code.
Merge requirement checklist