Skip to content
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 exporter: Standardize handling of attributes #3262

Merged
merged 22 commits into from
May 11, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,24 @@ option
* Fix handling of array-valued attributes for the OTLP trace exporter.
([#3238](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3238))

* Standardize on the set of data types supported for attribute values on
resources, metrics, and logs. The list of data types that must be supported
per the
[OpenTelemetry specification](https://github.com/open-telemetry/opentelemetry-specification/tree/main/specification/common#attribute)
is more narrow than what the .NET OpenTelemetry SDK supports. Supported data
types includes strings and most
[built-in value types](https://docs.microsoft.com/dotnet/csharp/language-reference/builtin-types/built-in-types)
with a few exceptions where overflow or rounding may occur (`ulong` and
`decimal`). Additionally, arrays of these data types are supported.
([#3262](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3262))

* **Breaking Change**: Previously, the OTLP exporter would `ToString()` any
`object`-valued attribute. This was a bug. In many circumstances `ToString()`
on an arbitrary type is not meaningful or worse could cause unintended, hard
to diagnose problems. Any type that is not explicitly supported is now
dropped.
([#3262](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3262))

## 1.3.0-beta.1

Released 2022-Apr-15
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,41 +201,6 @@ internal static OtlpTrace.Span ToOtlpSpan(this Activity activity)
return otlpSpan;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static OtlpCommon.KeyValue ToOtlpAttribute(this KeyValuePair<string, object> kvp)
{
if (kvp.Value == null)
{
return null;
}

var attrib = new OtlpCommon.KeyValue { Key = kvp.Key, Value = new OtlpCommon.AnyValue { } };

switch (kvp.Value)
{
case string s:
attrib.Value.StringValue = s;
break;
case bool b:
attrib.Value.BoolValue = b;
break;
case int i:
attrib.Value.IntValue = i;
break;
case long l:
attrib.Value.IntValue = l;
break;
case double d:
attrib.Value.DoubleValue = d;
break;
default:
attrib.Value.StringValue = kvp.Value.ToString();
break;
}

return attrib;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static OtlpTrace.Status ToOtlpStatus(this Activity activity, ref TagEnumerationState otlpTags)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
// <copyright file="OtlpCommonExtensions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using OtlpCommon = Opentelemetry.Proto.Common.V1;

namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation
{
internal static class OtlpCommonExtensions
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static OtlpCommon.KeyValue ToOtlpAttribute(this KeyValuePair<string, object> kvp)
{
if (kvp.Value == null)
{
return null;
}

var attrib = new OtlpCommon.KeyValue { Key = kvp.Key, Value = new OtlpCommon.AnyValue { } };

switch (kvp.Value)
{
case char:
case string:
attrib.Value.StringValue = Convert.ToString(kvp.Value);
break;
case bool b:
attrib.Value.BoolValue = b;
break;
case byte:
case sbyte:
case short:
case ushort:
case int:
case uint:
case long:
attrib.Value.IntValue = Convert.ToInt64(kvp.Value);
break;
case float:
case double:
attrib.Value.DoubleValue = Convert.ToDouble(kvp.Value);
break;
case Array array:
var arrayValue = attrib.Value.ArrayValue = new OtlpCommon.ArrayValue();
#pragma warning disable SA1011 // Closing square brackets should be spaced correctly
switch (kvp.Value)
{
case char[]:
case string[]:
foreach (var item in array)
{
arrayValue.Values.Add(new OtlpCommon.AnyValue { StringValue = Convert.ToString(item) });
}

break;
case bool[]:
foreach (var item in array)
{
arrayValue.Values.Add(new OtlpCommon.AnyValue { BoolValue = Convert.ToBoolean(item) });
}

break;
case byte[]:
case sbyte[]:
case short[]:
case ushort[]:
case int[]:
case uint[]:
case long[]:
#if NETSTANDARD || NETFRAMEWORK
if (array is nint[])
{
return null;
}
#endif
alanwest marked this conversation as resolved.
Show resolved Hide resolved
foreach (var item in array)
{
arrayValue.Values.Add(new OtlpCommon.AnyValue { IntValue = Convert.ToInt64(item) });
}

break;
case float[]:
case double[]:
foreach (var item in array)
{
arrayValue.Values.Add(new OtlpCommon.AnyValue { DoubleValue = Convert.ToDouble(item) });
}

break;
default:
return null;
}
#pragma warning restore SA1011 // Closing square brackets should be spaced correctly

break;

// case nint: Pointer type.
// case nuint: Pointer type.
// case ulong: May throw an exception on overflow.
// case decimal: Converting to double produces rounding errors.
default:
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a breaking change? as we were doing ToString() before, and now we ignore the value? should we retain the current behavior?
(Or we treat current behavior as a unintentional bug, and this is indeed the fix)

Either way - please add to changelog entry as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we treat current behavior as a unintentional bug, and this is indeed the fix

This is where I'm leaning but want to hear other opinions. While unusual, I have seen a ToString overload be susceptible to exceptions, mutating state, and even deadlock.

As an end user, I'd prefer to have to be intentional about how I'm ToStringing my attributes that are not just simple numeric types or strings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right change.

One thing to note is - do we log anywhere about this? i.e if I miss some values, and i enable self-diagnostics, would there be something that tells me exporter is dropping things?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At one point, I thought the Jaeger and Zipkin exporters did not ToString arbitrary attributes. I guess they do now. So, I'm ok ToStringing everything else if that's what folks prefer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note is - do we log anywhere about this? i.e if I miss some values, and i enable self-diagnostics, would there be something that tells me exporter is dropping things?

No logging at the moment - will add.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right change.

In that case, we should update Zipkin, Jaeger, Console, Prometheus to behave the same. Maybe there's some way to unify this logic, but I haven't put my mind to this yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From SIG meet: revert to use ToString(), and explore IFormattable in a follow up.

}

return attrib;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
// <copyright file="OtlpAttributeTests.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Collections.Generic;
using System.Linq;
using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation;
using Xunit;
using OtlpCommon = Opentelemetry.Proto.Common.V1;

namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests
{
public class OtlpAttributeTests
{
[Theory]
[InlineData(sbyte.MaxValue)]
[InlineData(byte.MaxValue)]
[InlineData(short.MaxValue)]
[InlineData(ushort.MaxValue)]
[InlineData(int.MaxValue)]
[InlineData(uint.MaxValue)]
[InlineData(long.MaxValue)]
[InlineData(new sbyte[] { 1, 2, 3 })]
[InlineData(new byte[] { 1, 2, 3 })]
[InlineData(new short[] { 1, 2, 3 })]
[InlineData(new ushort[] { 1, 2, 3 })]
[InlineData(new int[] { 1, 2, 3 })]
[InlineData(new uint[] { 1, 2, 3 })]
[InlineData(new long[] { 1, 2, 3 })]
public void IntegralTypesSupported(object value)
{
var kvp = new KeyValuePair<string, object>("key", value);
var attribute = kvp.ToOtlpAttribute();
Assert.NotNull(attribute);

switch (value)
{
case Array array:
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.ArrayValue, attribute.Value.ValueCase);
var expectedArray = new long[array.Length];
for (var i = 0; i < array.Length; i++)
{
expectedArray[i] = Convert.ToInt64(array.GetValue(i));
}

Assert.Equal(expectedArray, attribute.Value.ArrayValue.Values.Select(x => x.IntValue));
break;
default:
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.IntValue, attribute.Value.ValueCase);
Assert.Equal(Convert.ToInt64(value), attribute.Value.IntValue);
break;
}
}

[Theory]
[InlineData(float.MaxValue)]
[InlineData(double.MaxValue)]
[InlineData(new float[] { 1, 2, 3 })]
[InlineData(new double[] { 1, 2, 3 })]
public void FloatingPointTypesSupported(object value)
{
var kvp = new KeyValuePair<string, object>("key", value);
var attribute = kvp.ToOtlpAttribute();
Assert.NotNull(attribute);

switch (value)
{
case Array array:
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.ArrayValue, attribute.Value.ValueCase);
var expectedArray = new double[array.Length];
for (var i = 0; i < array.Length; i++)
{
expectedArray[i] = Convert.ToDouble(array.GetValue(i));
}

Assert.Equal(expectedArray, attribute.Value.ArrayValue.Values.Select(x => x.DoubleValue));
break;
default:
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.DoubleValue, attribute.Value.ValueCase);
Assert.Equal(Convert.ToDouble(value), attribute.Value.DoubleValue);
break;
}
}

[Theory]
[InlineData(true)]
[InlineData(new bool[] { true, false, true })]
public void BooleanTypeSupported(object value)
{
var kvp = new KeyValuePair<string, object>("key", value);
var attribute = kvp.ToOtlpAttribute();
Assert.NotNull(attribute);

switch (value)
{
case Array array:
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.ArrayValue, attribute.Value.ValueCase);
var expectedArray = new bool[array.Length];
for (var i = 0; i < array.Length; i++)
{
expectedArray[i] = Convert.ToBoolean(array.GetValue(i));
}

Assert.Equal(expectedArray, attribute.Value.ArrayValue.Values.Select(x => x.BoolValue));
break;
default:
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.BoolValue, attribute.Value.ValueCase);
Assert.Equal(Convert.ToBoolean(value), attribute.Value.BoolValue);
break;
}
}

[Theory]
[InlineData(char.MaxValue)]
[InlineData("string")]
public void StringTypesSupported(object value)
{
var kvp = new KeyValuePair<string, object>("key", value);
var attribute = kvp.ToOtlpAttribute();
Assert.NotNull(attribute);
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.StringValue, attribute.Value.ValueCase);
Assert.Equal(Convert.ToString(value), attribute.Value.StringValue);
}

[Fact]
public void StringArrayTypesSupported()
{
var charArray = new char[] { 'a', 'b', 'c' };
var stringArray = new string[] { "a", "b", "c" };

var kvp = new KeyValuePair<string, object>("key", charArray);
var attribute = kvp.ToOtlpAttribute();
Assert.NotNull(attribute);
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.ArrayValue, attribute.Value.ValueCase);
Assert.Equal(stringArray, attribute.Value.ArrayValue.Values.Select(x => x.StringValue));

kvp = new KeyValuePair<string, object>("key", stringArray);
attribute = kvp.ToOtlpAttribute();
Assert.NotNull(attribute);
Assert.Equal(OtlpCommon.AnyValue.ValueOneofCase.ArrayValue, attribute.Value.ValueCase);
Assert.Equal(stringArray, attribute.Value.ArrayValue.Values.Select(x => x.StringValue));
}

[Fact]
public void UnsupportedTypes()
{
var kvp = new KeyValuePair<string, object>("key", (nint)int.MaxValue);
var attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);

kvp = new KeyValuePair<string, object>("key", (nuint)uint.MaxValue);
attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);

kvp = new KeyValuePair<string, object>("key", decimal.MaxValue);
attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);

kvp = new KeyValuePair<string, object>("key", new object());
attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);

kvp = new KeyValuePair<string, object>("key", new nint[] { 1, 2, 3 });
attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);

kvp = new KeyValuePair<string, object>("key", new nuint[] { 1, 2, 3 });
attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);

kvp = new KeyValuePair<string, object>("key", new decimal[] { 1, 2, 3 });
attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);

kvp = new KeyValuePair<string, object>("key", new object[] { new object(), new object(), new object() });
attribute = kvp.ToOtlpAttribute();
Assert.Null(attribute);
}
}
}