fix: Don't set include_type_name query param for ES v7 template creation call

[AlexGoris-KasparSolutions]

What issue does this PR address?
#518

Does this PR introduce a breaking change?
No

Please check if the PR fulfils these requirements

• The commit follows our guidelines
• Unit Tests for the changes have been added (for bug fixes / features)
  Sorry, there are no unit tests around the changed code, and making them would require me to mock the ElasticLowLevelClient that's being used there, which would require more time then I can spend on this.

Other information:
The previous code (treat all <8 version the same) would have been fine if the TypeName would have had a valid value, but I noticed that in the constructor of the ElasticsearchSinkState class we have some code which sets its value to null for anything higher or equal to ES v7:

// Resolve typeName
if (_versionManager.EffectiveVersion.Major < 7)
    _options.TypeName = string.IsNullOrWhiteSpace(_options.TypeName)
        ? ElasticsearchSinkOptions.DefaultTypeName // "logevent"
        : _options.TypeName;
else
    _options.TypeName = null;

Therefor you had a scenario for ES v7 where the TypeName would be null, but the include_type_name query parameter would be set to true, which wasn't a valid API call for ES v7

[nenadvicentic]

This line is the the actual issue, because condition somehow got reversed from < to >= during refactoring for final 9.0.0:

serilog-sinks-elasticsearch/src/Serilog.Sinks.Elasticsearch/Sinks/ElasticSearch/ElasticsearchSinkState.cs

Line 55 in a01baff

private bool IncludeTypeName => _versionManager.EffectiveVersion.Major >= 7;

Could be fixed something like this (+ removing private member above completely):

StringResponse result;                
if (_versionManager.EffectiveVersion.Major < 8)
{ 
    result = _client.Indices.PutTemplateForAll<StringResponse>(_templateName, GetTemplatePostData(),
        new PutIndexTemplateRequestParameters
        {
            IncludeTypeName = _versionManager.EffectiveVersion.Major < 7 ? true : null
        });
}
else
{
    // Default to version 8 API
    result = _client.Indices.PutTemplateV2ForAll<StringResponse>(_templateName, GetTemplatePostData());
}

Or, if you prefer switch expression, please make it a bit simpler, so it's easier to read:

var result = _versionManager.EffectiveVersion.Major switch
{
    < 8 => _client.Indices.PutTemplateForAll<StringResponse>(_templateName, GetTemplatePostData(),
        new PutIndexTemplateRequestParameters
        {
            IncludeTypeName = _versionManager.EffectiveVersion.Major < 7 ? true : null
        }),
    // Default to version 8 API
    _ => _client.Indices.PutTemplateV2ForAll<StringResponse>(_templateName, GetTemplatePostData()),
};

It would also be great if you can add this check to unit tests after check for Path:

serilog-sinks-elasticsearch/test/Serilog.Sinks.Elasticsearch.Tests/Templating/Sendsv6TemplateTests.cs

Line 45 in a01baff

uri.AbsolutePath.Should().Be("/_template/serilog-events-template");

And in v7 and v8 to ensure that uri.Query should not contain include_type_name parameter.

[RefaelP23]

Any updates on this PR?
The regression introduced in the IncludeTypeName is breaking some of our logs to Elastic.

[nenadvicentic]

@RefaelP23 Yes, this was coding error. @AlexGoris-KasparSolutions did not yet reply to my review suggestions.
@mivano I believe this fix should be 9.0.1 release, before we switching to v10.

[RefaelP23]

@nenadvicentic Thanks for the prompt reply.

I also saw your discussion here Added original exception to FailureCallback action and I understand that it might take some time until it's released.
Is there some workaround I can try until it's released?

[nenadvicentic]

@RefaelP23 From the top of my head, I can only think of this two options:

1. Set configuration option AutoRegisterTemplate to false. I think this should suffice. I think Elasticsearch v7 is working just fine without using custom template.
2. Manually create index template on the Elasticsearch servers/clusters where you have the issue, using PostMan, curl, or similar tool. Perhaps to take exact template JSON from the servers where you currently don't have issue and put it to the once where you have the issue. You can list all templates via /_cat/templates and find the one related with this sink.

[mivano]

@nenadvicentic this one can be removed/closed now?

[nenadvicentic]

@mivano Yes. This can be deleted. I implemented in #533 what I described in this code review