serilog-sinks-elasticsearch icon indicating copy to clipboard operation
serilog-sinks-elasticsearch copied to clipboard

Can't configure BatchAction to ElasticOpType.Create using app settings

Open Originalutter opened this issue 5 years ago • 6 comments
trafficstars

A few questions before you begin:

Is this an issue related to the Serilog core project or one of the sinks or community projects.
This issue list is intended for Serilog Elasticsearch Sink issues. If this issue relates to another sink or to the code project, please log on the related repository. Please use Gitter chat and Stack Overflow for discussions and questions.

Does this issue relate to a new feature or an existing bug?

  • [x] Bug
  • [ ] New Feature

What version of Serilog.Sinks.Elasticsearch is affected? Please list the related NuGet package. <package id="Serilog.Sinks.Elasticsearch" version="8.4.0" targetFramework="net48" />

What is the target framework and operating system? See target frameworks & net standard matrix.

  • [ ] netCore 2.0
  • [ ] netCore 1.0
  • [x] 4.8
  • [ ] 4.7
  • [ ] 4.6.x
  • [ ] 4.5.x

Please describe the current behavior? AppSetting <add key="serilog:write-to:Elasticsearch.batchAction" value="create"/> causes application to crash during startup with the following message: System.ArgumentException: Requested value 'create' was not found. Changing the setting to <add key="serilog:write-to:Elasticsearch.batchAction" value="Create"/> makes the application run but my configuration does not seem to have BatchAction set to ElasticOpType.Create as my logging does not work (but works if I specify the configuration through code rather than from appsettings).

Please describe the expected behavior? AppSetting <add key="serilog:write-to:Elasticsearch.batchAction" value="create"/> should set BatchAction to ElasticOpType.Create in my sink configuration.

If the current behavior is a bug, please provide the steps to reproduce the issue and if possible a minimal demo of the problem Add the following app settings:

<add key="serilog:using" value="Serilog.Sinks.Elasticsearch"/>
<add key="serilog:write-to:Elasticsearch.nodeUris" value="https://nodeuri/"/>
<add key="serilog:write-to:Elasticsearch.batchAction" value="create"/>

Create a loggerconfiguration using Serilog.Settings.Appsettings nuget package with the following snippet new LoggerConfiguration().ReadFrom.AppSettings();

Originalutter avatar Sep 23 '20 20:09 Originalutter

Strange, not a case sensitive issue?

@orjan As you introduced this setting; did you ran into this?

mivano avatar Sep 24 '20 09:09 mivano

I'm looking into it as we speak together with @Originalutter

orjan avatar Sep 24 '20 10:09 orjan

There are a couple of issues here

The default doc_type is logevent if nothing else is specified e.g. so we'll need to set one the lines below:

<add key="serilog:write-to:Elasticsearch.typeName" value="_doc" />
<add key="serilog:write-to:Elasticsearch.typeName" value="" />

The docs is saying create with lowercase but the first character needs to be uppercase.

<add key="serilog:write-to:Elasticsearch.batchAction" value="Create"/>

~~When using the default data streams logs-*-* it's using a strict date parsing pattern for @timestamp and in https://github.com/serilog/serilog-sinks-elasticsearch/blob/dev/src/Serilog.Formatting.Elasticsearch/ElasticsearchJsonFormatter.cs#L285 we're just writing the value without any formatting. This will cause issues when running with a different timezone setting.~~

orjan avatar Sep 24 '20 12:09 orjan

datastream-date datastream-date_nano

I was confused by the error message, it's nothing wrong with the current date formatting, it works for both date and date_nano

orjan avatar Sep 24 '20 17:09 orjan

@mivano the remaining issue above is that it's not possible to remove _type by setting it to null via configuration. We can set it to _doc by setting it to _doc or ``

  1. It defaults to logevent

  2. Will be set to _doc

  3. When setting it to empty string it'll miss this condition

There are 2 options to remove the _type via configuration

  • Introduce a new property e.g. SuppressType
  • Let an empty string for typeName be the same as setting it to null so it'll be removed

The last one is probably the easiest way to implement it, but is also a breaking change for a few users. Looks a little bit further I'll guess we'll need to deprecate or remove everything related in the next Elastic major version.

What's your thoughts?

orjan avatar Sep 24 '20 18:09 orjan

At some point we simply need to drop support for older versions of ES and keep up with the new features. If that means breaking changes, that is fine, as long as they are documented.

mivano avatar Sep 28 '20 05:09 mivano