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

Resource attributes fix #1720

Merged
merged 4 commits into from
Jan 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@
* Added check in `ActivitySourceAdapter` class for root activity if traceid is
overridden by calling `SetParentId`
([#1355](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1355))
* Resource Attributes now accept int, short, and float as values, converting
them to supported data types (long for int/short, double for float). For
invalid attributes we now throw an exception instead of logging an error.
([#1720](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1720))

## 1.0.0-rc1.1

Expand Down
35 changes: 20 additions & 15 deletions src/OpenTelemetry/Resources/Resource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -101,28 +101,33 @@ private static KeyValuePair<string, object> SanitizeAttribute(KeyValuePair<strin
sanitizedKey = attribute.Key;
}

object sanitizedValue;
if (!IsValidValue(attribute.Value))
{
OpenTelemetrySdkEventSource.Log.InvalidArgument("Create resource", "attribute value", "Attribute value should be a non-null string, long, bool or double.");
sanitizedValue = string.Empty;
}
else
{
sanitizedValue = attribute.Value;
}

object sanitizedValue = SanitizeValue(attribute.Value, sanitizedKey);
return new KeyValuePair<string, object>(sanitizedKey, sanitizedValue);
}

private static bool IsValidValue(object value)
private static object SanitizeValue(object value, string keyName)
{
if (value != null && (value is string || value is bool || value is long || value is double))
if (value != null)
{
return true;
if (value is string || value is bool || value is long || value is double)
{
return value;
}

if (value is int || value is short)
{
return System.Convert.ToInt64(value);
}

if (value is float)
{
return System.Convert.ToDouble(value, System.Globalization.CultureInfo.InvariantCulture);
}

throw new System.ArgumentException("Attribute value type is not an accepted primitive", keyName);
}

return false;
throw new System.ArgumentException("Attribute value is null", keyName);
Copy link
Member

Choose a reason for hiding this comment

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

we need to have a follow up PR where we should throw if key is null as well.

}
}
}
32 changes: 14 additions & 18 deletions test/OpenTelemetry.Tests/Resources/ResourceTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,8 @@ public void CreateResource_NullAttributeValue()
// Arrange
var attributes = new Dictionary<string, object> { { "NullValue", null } };

// Act
var resource = new Resource(attributes);

// Assert
Assert.Single(resource.Attributes);

var attribute = resource.Attributes.Single();
Assert.Equal("NullValue", attribute.Key);
Assert.Empty((string)attribute.Value);
// Act and Assert
Assert.Throws<ArgumentException>(() => new Resource(attributes));
}

[Fact]
Expand Down Expand Up @@ -139,15 +132,25 @@ public void CreateResource_SupportedAttributeTypes()
{ "long", 1L },
{ "bool", true },
{ "double", 0.1d },

// int and float supported by conversion to long and double
{ "int", 1 },
{ "short", (short)1 },
{ "float", 0.1f },
};

var resource = new Resource(attributes);

Assert.Equal(4, resource.Attributes.Count());
Assert.Equal(7, resource.Attributes.Count());
Assert.Contains(new KeyValuePair<string, object>("string", "stringValue"), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("long", 1L), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("bool", true), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("double", 0.1d), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("int", 1L), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("short", 1L), resource.Attributes);

double convertedFloat = Convert.ToDouble(0.1f, System.Globalization.CultureInfo.InvariantCulture);
Assert.Contains(new KeyValuePair<string, object>("float", convertedFloat), resource.Attributes);
}

[Fact]
Expand All @@ -158,16 +161,9 @@ public void CreateResource_NotSupportedAttributeTypes()
{ "dynamic", new { } },
{ "array", new int[1] },
{ "complex", this },
{ "float", 0.1f },
};

var resource = new Resource(attributes);

Assert.Equal(4, resource.Attributes.Count());
Assert.Contains(new KeyValuePair<string, object>("dynamic", string.Empty), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("array", string.Empty), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("complex", string.Empty), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("float", string.Empty), resource.Attributes);
Assert.Throws<ArgumentException>(() => new Resource(attributes));
}

[Fact]
Expand Down