Add DD_TRACE_GLOBAL_TAGS Configuration Option #533
Conversation
bobuva
added
area:tracer
bobuva
changed the title
src/Datadog.Trace/Configuration/EnvironmentConfigurationSource.cs
Outdated
Show resolved
Hide resolved
src/Datadog.Trace/Configuration/EnvironmentConfigurationSource.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // 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("{")) |
There was a problem hiding this comment.
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 the JToken.Type instead.
| if (token.ToString().Contains("{")) | |
| if (token.Type == JTokenType.Object) |
There was a problem hiding this comment.
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.
@lucaspimentel if (token.Type == JTokenType.Object) won't work. The token.Type is String 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 a ToObject but instead always treat it as a string and use StringConfigurationSource.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.
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.
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 the JsonConfigurationSource constructor, as it should.
I also added a second bad data test.
| // 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.
Similar to above, token.ToString() serializes the token back into a json string, only to deserialize it a second time with JToken.Parse(). We should be able drop these two calls and use token.ToObject(...) directly.
| var dictionary = JToken.Parse(token.ToString()) | |
| var dictionary = token |
| if (token.ToString().Contains("{")) | ||
| { | ||
| var dictionary = JToken.Parse(token.ToString()) | ||
| ?.ToObject(typeof(ConcurrentDictionary<string, string>)) as ConcurrentDictionary<string, string>; |
There was a problem hiding this comment.
This one's more of a nit, but there's a generic version of ToObject<T> which doesn't require the cast, making this more concise.
| ?.ToObject(typeof(ConcurrentDictionary<string, string>)) as ConcurrentDictionary<string, string>; | |
| ?.ToObject<ConcurrentDictionary<string, string>>(); |
| 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.
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.
| @@ -90,7 +105,8 @@ public void DefaultSetting(Func<TracerSettings, object> settingGetter, object ex | |||
|
|
|||
| [Theory] | |||
| [MemberData(nameof(GetTestData))] | |||
| public void NameValueConfigurationSource1( | |||
| [MemberData(nameof(GetJsonTestData))] | |||
There was a problem hiding this comment.
Nice. I didn't know we could use [MemberData] multiple times. TIL.
|
|
||
| [Theory] | ||
| [MemberData(nameof(GetBadJsonTestData))] | ||
| public void JsonConfigurationSource_BadData( |
bobuva
removed
the
status:work-in-progress
| @@ -76,6 +78,9 @@ public TracerSettings(IConfigurationSource source) | |||
| false; | |||
|
|
|||
| Integrations = new IntegrationSettingsCollection(source); | |||
|
|
|||
| GlobalTags = source?.GetDictionary(ConfigurationKeys.GlobalTags) as IDictionary<string, string> ?? | |||
There was a problem hiding this comment.
nit: GetDictionary() already returns the correct type. No need to cast.
| GlobalTags = source?.GetDictionary(ConfigurationKeys.GlobalTags) as IDictionary<string, string> ?? | |
| GlobalTags = source?.GetDictionary(ConfigurationKeys.GlobalTags) ?? |
| if (token.Type == JTokenType.Object) | ||
| { | ||
| var dictionary = token | ||
| ?.ToObject<ConcurrentDictionary<string, string>>() as ConcurrentDictionary<string, string>; |
There was a problem hiding this comment.
nit: I think the as ConcurrentDictionary<string,string> is redundant with the parameterized ToObject call
There was a problem hiding this comment.
ah, yes! I just removed another of these earlier. Thanks for catching it!
There was a problem hiding this comment.
@zacharycmontoya I take that back, actually, the reason I put the as ConcurrentDictionary... in there is to avoid an exception being thrown. I think we need it there.
There was a problem hiding this comment.
We don't need the cast. The generic ToObject<T>() returns T, that cast is a no-op.
What exception were you expecting?
There was a problem hiding this comment.
If the cast fails at runtime, an InvalidCastException could be thrown. I don't know, maybe I'm wrong.
There was a problem hiding this comment.
Oh, I see. You're not wrong! We should probably wrap this in a try/catch in case the json is not valid (i.e. neither a string or an object).
But note that if the call to ToObject<T>() itself fails and throws an exception, that will happen whether we have that cast or not.
There was a problem hiding this comment.
The as <type> construction is intended to avoid the exception. It will return null instead of throwing. Can't seem to find the docs on it at the moment, but check out https://www.geeksforgeeks.org/c-sharp-as-operator-keyword/
You guys do a great job of reviewing code. Thanks for all of the good feedback! |
Addresses: Addition of a new configuration variable,
DD_TRACE_GLOBAL_TAGS, for setting global tags through environment. This is the first configuration setting whose value is a dictionary, with string key and string value.Rationale
These changes make the .NET tracer consistent with the other language tracers which already have the ability to add global tags.
Changes proposed in this pull request:
Configuration Settings
The
DD_TRACE_GLOBAL_TAGSconfiguration setting will now be recognized by the .NET Tracer, so that all new spans are tags with each of the entries in the configuration setting.@DataDog/apm-dotnet