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.

Sign up for GitHub

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 allow Primitive Arrays #1852

Merged
merged 13 commits into from
Mar 4, 2021
+90 −3
Merged

Resource Attributes allow Primitive Arrays #1852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5b7c1d8
Primitive arrays allowed in Resource Attributes + Tests
Austin-Tan Feb 25, 2021
e7b57d5
Changelog and spacing for build error
Austin-Tan Feb 25, 2021
870aa96
Fix Empty Value Test
Austin-Tan Feb 25, 2021
94940dc
fixing comprehensive supported types test
Austin-Tan Feb 25, 2021
e2b8c6a
Remove check for null key
Austin-Tan Feb 25, 2021
82beccb
Moving one test case to separate test method
Austin-Tan Feb 25, 2021
6761a93
add support for converted array types
Austin-Tan Feb 25, 2021
86c8fed
Remove null on key check
Austin-Tan Feb 25, 2021
f510661
Changelog merged fix
Austin-Tan Feb 25, 2021
af0f2ac
Merging
Austin-Tan Feb 25, 2021
f46020c
Adjusting test
Austin-Tan Feb 25, 2021
672fee3
completing changelog.md merge conflict
Austin-Tan Mar 1, 2021
314bc3f
merging changelog.md
Austin-Tan Mar 4, 2021
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
10 changes: 7 additions & 3 deletions src/OpenTelemetry/Resources/Resource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ private static KeyValuePair<string, object> SanitizeAttribute(KeyValuePair<strin
string sanitizedKey;
if (attribute.Key == null)
{
OpenTelemetrySdkEventSource.Log.InvalidArgument("Create resource", "attribute key", "Attribute key should be non-null string.");
sanitizedKey = string.Empty;
throw new System.ArgumentException("Resource's attributes contains a null key");
Copy link
Member Author

Choose a reason for hiding this comment

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

In response to:
https://github.com/open-telemetry/opentelemetry-dotnet/pull/1720/files#r564877372

Copy link
Member

Choose a reason for hiding this comment

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

The message might be misleading since it is talking about "attributes on the resource" but the method is only focusing on one attribute.
One possible way is to put something like nameof(attribute) has a null key here.

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 might be misunderstanding but each attribute is just a key and value, so a null key makes it impossible to do that right? I could print the Value instead since Key is null, but it may also be null and would then require checking and formatting

Copy link
Member Author

Choose a reason for hiding this comment

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

Found out C# prevents you from instantiating any dictionary where the key is null, it'll throw its own ArgumentNullException. Might be the reason we originally didn't check for key == null, and I'm happy to just remove this change if that makes sense.

}
else
{
Expand All @@ -109,7 +108,12 @@ private static object SanitizeValue(object value, string keyName)
{
if (value != null)
{
if (value is string || value is bool || value is long || value is double)
if (value is string || value is bool || value is double || value is long)
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
{
return value;
}

if (value is string[] || value is bool[] || value is double[] || value is long[])
Austin-Tan marked this conversation as resolved.
Show resolved Hide resolved
{
return value;
}
Expand Down
36 changes: 31 additions & 5 deletions test/OpenTelemetry.Tests/Resources/ResourceTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ public void CreateResource_NullAttributeValue()
Assert.Throws<ArgumentException>(() => new Resource(attributes));
}

[Fact]
public void CreateResource_NullAttributeKey()
{
// Arrange
var attributes = new Dictionary<string, object> { { null, "NullKey" } };

// Act and Assert
Assert.Throws<ArgumentException>(() => new Resource(attributes));
}

[Fact]
public void CreateResource_EmptyAttributeKey()
{
Expand All @@ -71,7 +81,11 @@ public void CreateResource_EmptyAttributeKey()
public void CreateResource_EmptyAttributeValue()
{
// Arrange
var attributes = new Dictionary<string, object> { { "EmptyValue", string.Empty } };
var attributes = new Dictionary<string, object>
{
{ "EmptyValue", string.Empty },
{ "EmptyArray", new string[0] },
};

// does not throw
var resource = new Resource(attributes);
Expand Down Expand Up @@ -129,28 +143,40 @@ public void CreateResource_SupportedAttributeTypes()
var attributes = new Dictionary<string, object>
{
{ "string", "stringValue" },
{ "long", 1L },
{ "bool", true },
{ "double", 0.1d },
{ "double", 0.1d},
{ "long", 1L },

// int and float supported by conversion to long and double
{ "int", 1 },
Austin-Tan marked this conversation as resolved.
Show resolved Hide resolved
{ "short", (short)1 },
{ "float", 0.1f },

// primitive array types supported
{ "string arr", new string[] { "stringValue" } },
{ "bool arr", new bool[] { true } },
{ "double arr", new double[] { 0.1d } },
{ "long arr", new long[] { 1L } },
};

var resource = new Resource(attributes);

Assert.Equal(7, resource.Attributes.Count());
Assert.Equal(11, 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>("long", 1L), 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);

Assert.Contains(new KeyValuePair<string, object>("string arr", new string[] { "stringValue" }), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("bool arr", new bool[] { true }), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("double arr", new double[] { 0.1d }), resource.Attributes);
Assert.Contains(new KeyValuePair<string, object>("long arr", new long[] { 1L }), resource.Attributes);
}

[Fact]
Expand Down