Extensibility prototype #129
Conversation
|
I took an initial look and the direction looks good 👍🏼 |
| 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 Path.Combine(GetCurrentDirectory(), "plugins.json") if nothing is provided.
I always fear such stuff because it is a great place that someone may use for ACE (Arbitrary Code Execution).
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 TryLoadJsonConfigurationFile). Just removing plugins.json defaults wouldn't give much benefits, as the ACE could be executed through autoloading datadog.json which in turn can direct to plugin by overloading unspecified plugin path.
var configurationFileName = configurationSource.GetString(ConfigurationKeys.ConfigurationFileName) ??
configurationSource.GetString("OTEL_DOTNET_TRACER_CONFIG_FILE") ??
Path.Combine(GetCurrentDirectory(), "datadog.json");
There was a problem hiding this comment.
👍 Then I would say the same mechanism be removed for datadog.json 😉
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.
I'm not 100% sure, that everything could be solved with env scoping or app.config / web.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'm not 100% sure, that everything could be solved with env scoping or app.config / web.config.
[...] Maybe just add another var to enable config load? - so by default it will be off.
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 plugins.json unlike the one used in the config should come with the install and be global for the whole machine. So it should follow logic similar to loading the integrations.json - if the plugins have configuration options per application those should be handled config (including the per-app json).
That said let's handle that in a separate PR.
…d multiple plugins.
I have a simple lab setup:
Main application makes request to API endpoint. That specific endpoint stores headers from the last request which can be accessed with another API endpoint. Here is the capture of the last requests with
|
| var plugins = PluginManager.TryLoadPlugins(GlobalSettings.Source.PluginsConfiguration); | ||
|
|
||
| // First call to create Tracer instace | ||
| Tracer.Instance = new Tracer(plugins); | ||
| } | ||
| catch | ||
| { |
There was a problem hiding this comment.
Not from your change but unless the logger can't work at this time it will be good to at least log the exception.
| 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.
The plugins.json unlike the one used in the config should come with the install and be global for the whole machine. So it should follow logic similar to loading the integrations.json - if the plugins have configuration options per application those should be handled config (including the per-app json).
That said let's handle that in a separate PR.
| { | ||
| var propagators = source.GetTypedValues<PropagatorType>(ConfigurationKeys.Propagators); | ||
| var propagators = source.GetStrings(ConfigurationKeys.Propagators); | ||
|
|
||
| if (!propagators.Any()) |
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.
| { | ||
| Log.Warning(ex, "Could not parse list of plugin paths. Invalid plugin configuration provided."); | ||
|
|
||
| return ArrayHelper.Empty<IOTelExtension>(); |
There was a problem hiding this comment.
Since this is a configuration file from a "distribution" my preference will be to fail to start as a whole.
| return ArrayHelper.Empty<IOTelExtension>(); | ||
| } | ||
|
|
||
| if (pluginFiles == null || !pluginFiles.Any()) |
There was a problem hiding this comment.
nit: since it is an array why use Any()? Just check the length.
| { | ||
| string fullPath = Path.GetFullPath(file); | ||
|
|
||
| if (File.Exists(fullPath)) |
There was a problem hiding this comment.
nit: the if is pretty big wrapping a try-catch a better if !file.Exists { log; continue; } makes the code easier to read.
| } | ||
| else | ||
| { | ||
| Log.Warning("Plugin path is defined but could not find the path '{0}'.", fullPath); |
There was a problem hiding this comment.
As mentioned somewhere else for plugins I think the right call is to fail the start.
|
I'm seeing a bunch of |
Background
Resolves #117
Plugins.json (default name) contains configuration for loadable plugin paths. Tried to use
GlobalSettingsto store plugin configuration andPluginManagerto encapsulate initialization logic.Requirements:
Examples:
What's next: