-
Notifications
You must be signed in to change notification settings - Fork 140
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
Add DD_TRACE_GLOBAL_TAGS Configuration Option #533
Changes from 9 commits
d5126d7
6eadd94
74841fd
e28ad1b
0cf8428
cdeecff
92d20a1
0879026
2f4048f
d8de2f2
bdb7126
b08499e
6812e7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,3 +1,5 @@ | ||||||
using System.Collections.Concurrent; | ||||||
using System.Collections.Generic; | ||||||
using System.IO; | ||||||
using Newtonsoft.Json; | ||||||
using Newtonsoft.Json.Linq; | ||||||
|
@@ -97,5 +99,40 @@ public T GetValue<T>(string key) | |||||
? default | ||||||
: token.Value<T>(); | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Gets a <see cref="ConcurrentDictionary{TKey, TValue}"/> containing all of the values. | ||||||
/// </summary> | ||||||
/// <remarks> | ||||||
/// Example JSON where `globalTags` is the configuration key. | ||||||
/// { | ||||||
/// "globalTags": { | ||||||
/// "name1": "value1", | ||||||
/// "name2": "value2" | ||||||
/// } | ||||||
/// } | ||||||
/// </remarks> | ||||||
/// <param name="key">The key that identifies the setting.</param> | ||||||
/// <returns><see cref="IDictionary{TKey, TValue}"/> containing all of the key-value pairs.</returns> | ||||||
/// <exception cref="JsonReaderException">Thrown if the configuration value is not a valid JSON string.</exception>" | ||||||
public IDictionary<string, string> GetDictionary(string key) | ||||||
{ | ||||||
var token = _configuration.SelectToken(key, errorWhenNoMatch: false); | ||||||
if (token == null) | ||||||
{ | ||||||
return null; | ||||||
} | ||||||
|
||||||
// Presence of a curly brace indicates that this is likely a JSON string. An exception will be thrown | ||||||
// if it fails to parse or convert to a dictionary. | ||||||
if (token.ToString().Contains("{")) | ||||||
{ | ||||||
var dictionary = JToken.Parse(token.ToString()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above,
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||||||
?.ToObject(typeof(ConcurrentDictionary<string, string>)) as ConcurrentDictionary<string, string>; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one's more of a nit, but there's a generic version of
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||||||
return dictionary; | ||||||
} | ||||||
|
||||||
return StringConfigurationSource.ParseCustomKeyValues(token.ToString()); | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,14 +6,8 @@ | |
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<None Include="..\..\src\Datadog.Trace.ClrProfiler.Native\bin\$(Configuration)\$(Platform)\**" | ||
CopyToOutputDirectory="Always" | ||
CopyToPublishDirectory="Always" | ||
Link="profiler-lib\%(RecursiveDir)\%(Filename)%(Extension)" /> | ||
<Content Include="..\..\integrations.json" | ||
CopyToOutputDirectory="Always" | ||
CopyToPublishDirectory="Always" | ||
Link="profiler-lib\integrations.json" /> | ||
<None Include="..\..\src\Datadog.Trace.ClrProfiler.Native\bin\$(Configuration)\$(Platform)\**" CopyToOutputDirectory="Always" CopyToPublishDirectory="Always" Link="profiler-lib\%(RecursiveDir)\%(Filename)%(Extension)" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we revert this entire files? Looks like VS added some json schema stuff at the end and also reformatted this line. I assume this happened while you were testing things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
<Content Include="..\..\integrations.json" CopyToOutputDirectory="Always" CopyToPublishDirectory="Always" Link="profiler-lib\integrations.json" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
|
@@ -24,4 +18,6 @@ | |
<ItemGroup> | ||
<PackageReference Include="Newtonsoft.Json" Version="12.0.1" /> | ||
</ItemGroup> | ||
|
||
<ProjectExtensions><VisualStudio><UserProperties _1_1_4_1_1_4integrations_1json__JsonSchema="http://json.schemastore.org/ansible-stable-2.7" /></VisualStudio></ProjectExtensions> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ public static IEnumerable<object[]> GetDefaultTestData() | |
yield return new object[] { CreateFunc(s => s.ServiceName), null }; | ||
yield return new object[] { CreateFunc(s => s.DisabledIntegrationNames.Count), 0 }; | ||
yield return new object[] { CreateFunc(s => s.LogsInjectionEnabled), false }; | ||
yield return new object[] { CreateFunc(s => s.GlobalTags.Count), 0 }; | ||
} | ||
|
||
public static IEnumerable<object[]> GetTestData() | ||
|
@@ -36,6 +37,20 @@ public static IEnumerable<object[]> GetTestData() | |
yield return new object[] { ConfigurationKeys.ServiceName, "web-service", CreateFunc(s => s.ServiceName), "web-service" }; | ||
|
||
yield return new object[] { ConfigurationKeys.DisabledIntegrations, "integration1;integration2", CreateFunc(s => s.DisabledIntegrationNames.Count), 2 }; | ||
|
||
yield return new object[] { ConfigurationKeys.GlobalTags, "k1:v1, k2:v2", CreateFunc(s => s.GlobalTags.Count), 2 }; | ||
} | ||
|
||
// JsonConfigurationSource needs to be tested with JSON data, which cannot be used with the other IConfigurationSource implementations. | ||
public static IEnumerable<object[]> GetJsonTestData() | ||
{ | ||
yield return new object[] { ConfigurationKeys.GlobalTags, @"{ ""name1"":""value1"", ""name2"": ""value2""}", CreateFunc(s => s.GlobalTags.Count), 2 }; | ||
} | ||
|
||
public static IEnumerable<object[]> GetBadJsonTestData() | ||
{ | ||
// Extra opening brace | ||
yield return new object[] { ConfigurationKeys.GlobalTags, @"{ {""name1"":""value1"", ""name2"": ""value2""}" }; | ||
} | ||
|
||
public static Func<TracerSettings, object> CreateFunc(Func<TracerSettings, object> settingGetter) | ||
|
@@ -90,7 +105,8 @@ public void EnvironmentConfigurationSource( | |
|
||
[Theory] | ||
[MemberData(nameof(GetTestData))] | ||
public void NameValueConfigurationSource1( | ||
[MemberData(nameof(GetJsonTestData))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. I didn't know we could use |
||
public void JsonConfigurationSource( | ||
string key, | ||
string value, | ||
Func<TracerSettings, object> settingGetter, | ||
|
@@ -104,5 +120,18 @@ public void NameValueConfigurationSource1( | |
object actualValue = settingGetter(settings); | ||
Assert.Equal(expectedValue, actualValue); | ||
} | ||
|
||
[Theory] | ||
[MemberData(nameof(GetBadJsonTestData))] | ||
public void JsonConfigurationSource_BadData( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice test! 👍 |
||
string key, | ||
string value) | ||
{ | ||
var config = new Dictionary<string, string> { [key] = value }; | ||
string json = JsonConvert.SerializeObject(config); | ||
IConfigurationSource source = new JsonConfigurationSource(json); | ||
|
||
Assert.Throws<JsonReaderException>(() => { new TracerSettings(source); }); | ||
} | ||
} | ||
} |
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.
At this point the json library already deserialized the json string, so I don't think we should serialize it back to a json string by calling
JToken.ToString()
. Consider checking theJToken.Type
instead.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.
oh that's interesting. I was looking for a
JToken
method or property to validate, but that looks good! Thanks I'll test it out.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.
@lucaspimentel
if (token.Type == JTokenType.Object)
won't work. Thetoken.Type
isString
even in the case where the config string contains valid JSON, so it is never converted. What might work best is to not convert it using aToObject
but instead always treat it as a string and useStringConfigurationSource.ParseCustomKeyValues
all of the time. This has the side-benefit of being able to handle the case of an extra curly brace in the config.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.
After more thought, I'm inclined to continue to try to verify that the string is JSON by checking for the
{
character. Since this is considered to be a JSON-formatted config string, that appears to be the only way to verify it, at least that I can find. What I said above about always treating it like a string is not a good idea because bad JSON, e.g., including extra braces, will result in the keys and/or values including the braces, which would be a bad thing.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.
Ok, this has finally been improved (thanks for the insight, @lucaspimentel ). Now the tests for
JsonConfigurationSource
do not start by putting the key/value data into a dictionary. Instead, the value is passed directly to theJsonConfigurationSource
constructor, as it should.I also added a second bad data test.