markmcdowell / NLog.Targets.ElasticSearch

NLog target for Elasticsearch
MIT License
176 stars 89 forks source link

Setting the URI of the Target #83

Closed babnik42 closed 5 years ago

babnik42 commented 5 years ago

I'm having a little trouble setting the URI for different environments. So if my target has no Uri or ConnectionStringName property, then running locally is fine. Default is http://localhost:9200 and it works. Now when I run in a container, I need to change that Uri to http://elasticsearch:9200. If I manually set the Uri property on the target, it works. I however would like to use appsettings.json and specifically the environment specific settings files, appsettings.Development.json and appsettings.Production.json. So I remove the Uri property from the target, and replace it with ConnectionStringName, set to ElasticsearchUrl. I then add a connection string to each of my appsettings files.

<target name="Elasticsearch" xsi:type="BufferingWrapper" flush Timeout="5000">
  <target xsi:type="Elasticsearch"
          ConnectionStringName="ElasticsearchUrl"
          IncludeAllProperties="true"
          Index="log-${date:format=yyyy.mm.dd}">
  </target>
</target>

And appsettings :

{
  "ConnectionStrings":{
    "ElasticsearchUrl": "http://elasticsearch:9200"
  }
}

This doesn't get loaded. And even if I deliberately change the development setting to something wrong it keeps on sending it to the default. What an I doing wrong?

markmcdowell commented 5 years ago

The code is quite simple here, see below, could it be a directory issue? I can see it's doing Directory.GetCurrentDirectory() which is the same as the dll directory.

var builder = new ConfigurationBuilder()
    .SetBasePath(Directory.GetCurrentDirectory())
    .AddJsonFile("appsettings.json", true, reloadOnChange: false);

var configuration = builder.Build();

return configuration.GetConnectionString(name);
babnik42 commented 5 years ago

Ok, that explains it. There is no provision for environment specific appsettings which is what I was trying to do. This is what would be needed

var builder = new ConfigurationBuilder()
    .SetBasePath(Directory.GetCurrentDirectory())
    .AddJsonFile("appsettings.json", true, reloadOnChange: false)
    .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true, reloadOnChange: false); // optional extra provider
var configuration = builder.Build();

return configuration.GetConnectionString(name);`

where env is the HostingEnvironment. If I move the connection string to appsettings.json then it would work. As it is I have them in appsettings.Development.json and appsettings.Production.json. However moving it would defeat the purpose of having an environment specific setting.

babnik42 commented 5 years ago

It should possibly also add a line to load from environment variables

var builder = new ConfigurationBuilder()
    .SetBasePath(Directory.GetCurrentDirectory())
    .AddJsonFile("appsettings.json", true, reloadOnChange: false)
    .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true, reloadOnChange: false); // optional extra provider
builder.AddEnvironmentVariables();
var configuration = builder.Build();

return configuration.GetConnectionString(name);

That way the connectionStringName can be overridden by an environment variable in a docker compose file as well as being setup in the app settings.

snakefoot commented 5 years ago

@babnik42 Consider making use of ${configsetting} for Uri to lookup the setting. See also https://github.com/NLog/NLog/wiki/ConfigSetting-Layout-Renderer

P.S. You have to update <extensions> until NLog 4.6 gets out:

<extensions>
    <add assembly="NLog.Extensions.Logging"/>
</extensions>
markmcdowell commented 5 years ago

I think using the nlog extensions mention by @snakefoot is the way to go on this and we'll end up removing the code reading appsettings.json in a future release.

babnik42 commented 5 years ago

Yes, that works and is a better solution.