Extensibility prototype #129
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,11 @@ internal GlobalSettings(IConfigurationSource source) | |
| DiagnosticSourceEnabled = source?.GetBool(ConfigurationKeys.DiagnosticSourceEnabled) ?? | ||
| // default value | ||
| true; | ||
|
|
||
| if (TryLoadPluginJsonConfigurationFile(source, out JsonConfigurationSource jsonConfigurationSource)) | ||
| { | ||
| PluginsConfiguration = jsonConfigurationSource; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -55,6 +60,11 @@ internal GlobalSettings(IConfigurationSource source) | |
| /// </summary> | ||
| internal bool DiagnosticSourceEnabled { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the plugins configuration. | ||
| /// </summary> | ||
| internal JsonConfigurationSource PluginsConfiguration { get; } | ||
|
|
||
| /// <summary> | ||
| /// Set whether debug mode is enabled. | ||
| /// Affects the level of logs written to file. | ||
|
|
@@ -121,15 +131,28 @@ internal static CompositeConfigurationSource CreateDefaultConfigurationSource() | |
| return configurationSource; | ||
| } | ||
|
|
||
| private static bool TryLoadPluginJsonConfigurationFile(IConfigurationSource configurationSource, out JsonConfigurationSource jsonConfigurationSource) | ||
| { | ||
| var configurationFileName = configurationSource?.GetString(ConfigurationKeys.PluginConfigurationFileName) ?? | ||
| Path.Combine(GetCurrentDirectory(), "plugins.json"); | ||
|
There was a problem hiding this comment. Not sure if we want to use The bad-guy must only have a possibility to create some file in the same directory (it can be created by a different user!). The user may be not aware of the risk. I would prefer to require explicitly enabling via ENV VAR. Non-admin user is unable to set env var for different users. There was a problem hiding this comment. Initially almost the same logic as for json configuration file, just tries to load different file (see There was a problem hiding this comment. 👍 Then I would say the same mechanism be removed for There was a problem hiding this comment. Sounds reasonable. Should we refactor json configuration loading in different PR? - as this addresses completely different logic than current one. + the parsing issue #129 (comment). There was a problem hiding this comment. Did you not mean #129 (comment) ? For me, it can be a separate PR + GH issue (just to not forget about it) There was a problem hiding this comment. We had the issue with IIS hosting multiple .NET Framework web apps, that could not be env scoped - although this issue was solved with web.config / app.config. Maybe just add another var to enable config load? - so by default it will be off. There was a problem hiding this comment.
I would rather add a var to enable config load if we are 100% sure that we cannot solve it any other way. Less is more 😉 There was a problem hiding this comment. I would agree if it's about adding functionality. But more concerned when we are reducing publicly available functionality. Anyway we can discuss it under a new issue when it's open and more detailed. There was a problem hiding this comment. I am ok with moving it to an issue and discussing it during SIG. See #129 (comment) 😉 There was a problem hiding this comment. The That said let's handle that in a separate PR. |
||
|
|
||
| return TryLoadJsonConfigurationFile(configurationFileName, out jsonConfigurationSource); | ||
| } | ||
|
|
||
| private static bool TryLoadJsonConfigurationFile(IConfigurationSource configurationSource, out JsonConfigurationSource jsonConfigurationSource) | ||
| { | ||
| // if environment variable is not set, look for default file name in the current directory | ||
| var configurationFileName = configurationSource.GetString(ConfigurationKeys.ConfigurationFileName) ?? | ||
| configurationSource.GetString("OTEL_DOTNET_TRACER_CONFIG_FILE") ?? | ||
| Path.Combine(GetCurrentDirectory(), "datadog.json"); | ||
|
|
||
| return TryLoadJsonConfigurationFile(configurationFileName, out jsonConfigurationSource); | ||
| } | ||
|
|
||
| private static bool TryLoadJsonConfigurationFile(string configurationFileName, out JsonConfigurationSource jsonConfigurationSource) | ||
| { | ||
| try | ||
| { | ||
| if (string.Equals(Path.GetExtension(configurationFileName), ".JSON", StringComparison.OrdinalIgnoreCase) && | ||
| File.Exists(configurationFileName)) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text.RegularExpressions; | ||
| using Datadog.Trace.Configuration.Types; | ||
| using Datadog.Trace.ExtensionMethods; | ||
| using Datadog.Trace.PlatformHelpers; | ||
| using Datadog.Trace.Util; | ||
|
|
@@ -369,7 +370,7 @@ public TracerSettings(IConfigurationSource source) | |
| /// Default is <c>Datadog</c> | ||
| /// <seealso cref="ConfigurationKeys.Propagators"/> | ||
| /// </summary> | ||
| public HashSet<string> Propagators { get; set; } | ||
pellared marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary> | ||
| /// Gets or sets a value indicating whether runtime metrics | ||
|
|
@@ -640,18 +641,18 @@ internal string GetServiceName(Tracer tracer, string serviceName) | |
| return ServiceNameMappings.GetServiceName(tracer.DefaultServiceName, serviceName); | ||
| } | ||
|
|
||
| private static HashSet<string> GetPropagators(IConfigurationSource source) | ||
| { | ||
| var propagators = source.GetStrings(ConfigurationKeys.Propagators); | ||
|
|
||
| if (!propagators.Any()) | ||
|
There was a problem hiding this comment. This will cause multiple enumeration (at There was a problem hiding this comment. Originally it was intentional due perf and allocation benefits (Enumerate first + enumerate all vs allocate new list / array). @pellared any ideas which way is more preferable? There was a problem hiding this comment. I would not worry much about the perf for such cases since it is executed once per application lifetime. Of course, I'm not saying to ignore performance and to not do easy/obvious savings but the impact here will be very minimal anyway. |
||
| { | ||
| // TODO: Default to W3C (be aware of integration tests) | ||
| // see more: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#global-propagators | ||
| return new HashSet<string>() { PropagatorTypes.Datadog }; | ||
| } | ||
|
|
||
| return new HashSet<string>(propagators); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| namespace Datadog.Trace.Configuration.Types | ||
| { | ||
| /// <summary> | ||
| /// Contains default available propagator types. | ||
| /// </summary> | ||
| public static class PropagatorTypes | ||
| { | ||
| /// <summary> | ||
| /// The Datadog propagator. | ||
| /// </summary> | ||
| public const string Datadog = "Datadog"; | ||
|
|
||
| /// <summary> | ||
| /// The B3 propagator | ||
| /// </summary> | ||
| public const string B3 = "B3"; | ||
|
|
||
| /// <summary> | ||
| /// The W3C propagator | ||
| /// </summary> | ||
| public const string W3C = "W3C"; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| namespace Datadog.Trace.Plugins | ||
| { | ||
| /// <summary> | ||
| /// Base marker interface for extendability points. | ||
| /// </summary> | ||
| public interface IOTelExtension | ||
| { | ||
| } | ||
| } |
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.
Not from your change but unless the logger can't work at this time it will be good to at least log the exception.