Updating exporters to use TagObjects instead of Tags

[eddynaka]

Fixes part of #940

Part 2 of #954

Changes

Please provide a brief description of the changes here. Update the
CHANGELOG.md for non-trivial changes.

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

• Design discussion issue #
• Changes in public API reviewed

[cijothomas]

can you add my open qn here: what is the Value is of some other type? (eg: Datetime). Should we try to call .ToString() on it? or drop it?

[eddynaka]

i think we have a duplicate part of code. let me change and we can review again.

[eddynaka]

Right now, we already have a method that converts the default case to string:

        public static JaegerTag ToJaegerTag(this KeyValuePair<string, object> attribute)
        {
            return attribute.Value switch
            {
                string s => new JaegerTag(attribute.Key, JaegerTagType.STRING, vStr: s),
                int i => new JaegerTag(attribute.Key, JaegerTagType.LONG, vLong: Convert.ToInt64(i)),
                long l => new JaegerTag(attribute.Key, JaegerTagType.LONG, vLong: l),
                float f => new JaegerTag(attribute.Key, JaegerTagType.DOUBLE, vDouble: Convert.ToDouble(f)),
                double d => new JaegerTag(attribute.Key, JaegerTagType.DOUBLE, vDouble: d),
                bool b => new JaegerTag(attribute.Key, JaegerTagType.BOOL, vBool: b),
                _ => new JaegerTag(attribute.Key, JaegerTagType.STRING, vStr: attribute.Value.ToString()),
            };
        }

[alanwest]

can you add my open qn here: what is the Value is of some other type? (eg: Datetime). Should we try to call .ToString() on it? or drop it?

This is an interesting question. Has there been any efforts you're aware of at the spec level for this? I like being able to support DateTime versus dropping it, but dates are always hard, particularly handling them across languages. Does it make sense to adopt ISO 8601 as a standard format? I think in .NET that is done with .ToString("o").

TimeSpan can be useful too. I think by default ToString formats these as HH:MM:SS. Does it make sense to just adopt seconds or milliseconds?

[cijothomas]

I am not aware of any spec changes coming to support more types for attributes.
Span and SpanEvent has timestamps on it, so probably there is no need of supporting tag of type datetime ?

If one is using OTel API, they can only send supported types - string, int, bool, array. Not possible to send DateTime or any other type.
(Activity support string,object pair, so we need to decide if the exporters should do "ToString" on whatever custom objects are provided)

[eddynaka]

Right now, the default is ToString, we could close this and open a new issue specific for other types. What do u think? @cijothomas @alanwest

[cijothomas]

Merging this now - we need to comeback and decide if we are okay to do .ToString here.

[codecov]

Codecov Report

Merging #1000 into master will increase coverage by 0.02%.
The diff coverage is 68.08%.

@@            Coverage Diff             @@
##           master    #1000      +/-   ##
==========================================
+ Coverage   68.19%   68.22%   +0.02%     
==========================================
  Files         219      219              
  Lines        5974     5976       +2     
  Branches      978      978              
==========================================
+ Hits         4074     4077       +3     
+ Misses       1626     1622       -4     
- Partials      274      277       +3     

Impacted Files	Coverage Δ
...metry.Exporter.Zipkin/Implementation/ZipkinSpan.cs	72.09% <21.42%> (-10.34%)	⬇️
...plementation/ZipkinActivityConversionExtensions.cs	78.94% <82.35%> (+4.75%)	⬆️
....Jaeger/Implementation/JaegerActivityExtensions.cs	89.39% <93.33%> (+4.77%)	⬆️
...metryProtocol/Implementation/ActivityExtensions.cs	87.70% <100.00%> (+0.20%)	⬆️

[cijothomas]

we now need to deal with the array of primitives since #999 is merged.

[cijothomas]

Okay to have separate PR for handling array of primitives.

[eddynaka]

Okay to have separate PR for handling array of primitives.

About that, if we receive an array of primitives, should we transmite that? how are we going to handle?
But, sure, i agree that they are different things and we can separate.

I'm asking just to create an issue so we can track :)

[CodeBlanch]

@tarekgh Just noticing Activity.TagObjects uses a custom LinkedList and not ActivityTagsCollection like ActivityEvent & ActivityLink use. Because of that, no struct enumerator, perf regression for us anytime we enumerate TagObjects. Is there a compelling reason for the difference? What we really need is for whatever is underneath the IEnumerable returned to have a struct GetEnumerator() operation so we can foreach without allocation.

Something like...

   private class TagsLinkedList : IEnumerable<KeyValuePair<string, object>>
   {
        public Enumerator<KeyValuePair<string, object>> GetEnumerator() { return Enumerator(this); }
        IEnumerator<T> IEnumerable<KeyValuePair<string, object>>.GetEnumerator() { return Enumerator(this); }
        IEnumerator IEnumerable.GetEnumerator() { return Enumerator(this); }

        private struct Enumerator<KeyValuePair<string, object>> { ... }
   }

        public IEnumerable<KeyValuePair<string, object?>> TagObjects
        {
            get => _tags ?? Unsafe.As<IEnumerable<KeyValuePair<string, object?>>>(s_emptyBaggageTags);
        }

[tarekgh]

This is something can look at to optimize as everything is implementation details. I didn't change anything from what we used to have. Note that this will be a little bit tricky because it is possible some tags get removed while someone else enumerating the list.

[CodeBlanch]

Awesome this would help a lot. When I say perf regression I mean moving from our old Span object to using Activity to drive everything.

[tarekgh]

If you are interested, you may submit a PR for that? we can work out the details together if you want.

CC @noahfalk

[CodeBlanch]

@tarekgh Done dotnet/runtime#40362