Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

Automatically handle TypeName parameter for different versions of Elasticsearch (<7, 7 or higher) #462

Merged
merged 28 commits into from
Jan 23, 2023
Merged

Automatically handle TypeName parameter for different versions of Elasticsearch (<7, 7 or higher) #462

merged 28 commits into from
Jan 23, 2023

Conversation

nenadvicentic
Copy link
Contributor

What issue does this PR address?

  • Changes default TypeName behavior to fit compatibility with Elasticsearch version 7, 8 and higher, instead of 6 or lower.
  • When options ElasticsearchSinkOptions.DetectElasticsearchVersion is set to true handles automatically compatibility of TypeName parameter (_type in JSON body) against Elasticsearch version 7 or higher, as well version 6 or lover.

Does this PR introduce a breaking change?
Yes. Default behavior, settings TypeName="logevent" unless TypeName has been set to null explicitly, has changed. TypeName remains null, if it has not been set manually. This fits with behavior expected by Elasticsearch version 7 or higher.

Please check if the PR fulfills these requirements

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)

@nenadvicentic nenadvicentic changed the title Feature/automatically handle typename Automatically handle TypeName parameter for different Elasticsearch version (<7, 7 or higher) Aug 17, 2022
@nenadvicentic nenadvicentic changed the title Automatically handle TypeName parameter for different Elasticsearch version (<7, 7 or higher) Automatically handle TypeName parameter for different versions of Elasticsearch (<7, 7 or higher) Aug 17, 2022
@nenadvicentic
Copy link
Contributor Author

@mivano @jonasmidstrup - Hi guys, when you have time, can you take a look into the PR?

Technically, this is going in a good direction, but I think it needs a few more things to be better rounded update.

  1. Index Template for version Elasticserach v8
  2. Maybe more clear option name to describe the behavior, instead of already existing DetectElasticsearchVersion.
  3. To properly name and categorize unit and integration tests I've added.

Regarding unit-tests,, I added two of those the way I'm approaching naming, etc. One is, what I consider proper unit-test, another is actually integration test, but does not fit with logic of previous integration tests in this project. So, there I could use some guidance on conventions/intentions.

@jonasmidstrup
Copy link

jonasmidstrup commented Sep 26, 2022

@nenadvicentic Think it looks good 👍🏻 Have you tried it with different Elasticsearch versions?

I think it would be great to have an actual index template for v8.

@nenadvicentic
Copy link
Contributor Author

nenadvicentic commented Sep 26, 2022

@jonasmidstrup I tried this on Elasticsearch versions 7 and 8.

Regarding template for v8, I will take a look in next days, if I can figure out how to make working template, but this is a new topic for me.

nenadvicentic and others added 19 commits January 13, 2023 07:51
* last version of Elasticsearch that supported user-defined `_type` field - v6.8.x is running out of support on 2022-02-10 (https://www.elastic.co/support/eol)
…t versions Elasticsearch (<7, 7+) when `ElasticsearchSinkOptions.DetectElasticsearchVersion` is enabled.
…csearchVersion` is set to `true`.

- Two methods used - instantiate `ElasticsearchSinkState` directly and via `LoggerConfiguration`
…n to `int` + decision branch for which version of index-template API to use + removal of obsolete `ElasticsearchTemplateProvider.GetTemplate(...)` method overload.
…csearchSink` so that it does not use obsolete `PeriodicBatchingSink` constructor.
@nenadvicentic
Copy link
Contributor Author

nenadvicentic commented Jan 20, 2023

@jonasmidstrup @mivano Guys, I am done with changes concerning this task. Please take a look when you have time.

Most important changes are:

  • DetectElasticsearchVersion is now set to true by default. This helps determine how to handle TypeName across different major versions of elasticsearch and it also helps deciding which URL to use when uploading index template (v8 has new endpoint for this: _index_template vs old template.

  • TypeName is null by default (although it is also touched once Elasticsearch version is detected).

  • Internal class ElasticsearchVersionManager has been added, mainly to handle situations where detection of version fails or when it is disabled. In the case of fallback, sink will assume "default" version 7.

    Reasoning to use version 7 as a default is that Elasticsearch.NET client version 7.15.2 supports major versions of Elasticsearch server from 6 to 8 and that version 8 has "minimum_wire_compatibility_version" : "7.17.0" and "minimum_index_compatibility_version" : "7.0.0", so version 7 has broadest compatibility at the moment.

Other changes:

  • Nuget pacakges have been updated (except for the Elasticsearch integration-tests related packages)
  • Most of the ElasticserachSink functionality has been moved into internal BatchedElasticsearchSink class that inherits from IBatchedLogEventSink, so it complies with new recommended way of integration with PeriodicBatchingSink and we don't use obsolete constructors.
  • ConnectionStub was moved out of ElasticsearchSinkTestsBase and extended. Both are now in /Stubs subfolder. Newer versions of Elasticsearch.NET client are now using "pre-flight" request to determine if endpoint is Elasticsearch and if it is indeed between 6 and 8. ConnectionStub had to accommodate for that.
  • Unit tests have been fixed/added accordingly.

I have tested this version of code against Elasticsearch 7 and 8, using minimal configuration:

  "Serilog": {
    "Using": [ "Serilog.Sinks.Elasticsearch" ],
    "MinimumLevel": "Information",
    "WriteTo": [
      {
        "Name": "Elasticsearch",
        "Args": {
          "nodeUris": "http://iutlog1:9200"
        }
      }
    ]
  }

Regarding Serilog.Sinks.Elasticsearch.IntegrationTests project, I initially tried to update Elastic.Elasticsearch.Xunit and Elastic.Elasticsearch.Managed, but left the tests as they were, as I wasn't able to make the tests working at all. They are also breaking on GitHub (at least in my fork), due to missing HTTP resources. This is completely out of my depth, so I would not spend more time tinkering with this project

@mivano
Copy link
Contributor

mivano commented Jan 20, 2023

Thanks @nenadvicentic for your efforts! Great to see some improvements and needed updates. Unfortunately the tests are tricky. I will try to find some time to see if I can get them to work. Furthermore, we need to make sure we document the changes, preferably in the changes.md and readme.md file.

@nenadvicentic
Copy link
Contributor Author

nenadvicentic commented Jan 23, 2023

@mivano I updated both ReadMe.md and Changes.md to reflect the changes.

Also, I investigated tests issue a bit deeper, including updating GitHubActionsTestLogger NuGet package, which gave a bit clearer messages.

Firstly, integration tests are not even run on GitHub CI. Unit tests are the once throwing error.

I think updating .github/workflows/cicd.yaml file would fix current issue (but even if it does not fix the issues, we should update SDK):

      - name: Install .NET SDK
        uses: actions/setup-dotnet@v3.0.3
        with:
-          dotnet-version: '3.1.x'
+          dotnet-version: '7.0.x'

However, I do not have permissions to do that.

My suggestion to you is that we merge this PR sooner, rather than later to the dev branch, since it contains quite a lot of code changes and cleaning up. We cannot make this Sink perfect with a single PR and we should keep this PR on the topic (solving problem of TypeName). Once this is merged, we make further improvements from there, and experiment with feature branches (e.g. for Elastic Common Schema topic).

@mivano
Copy link
Contributor

mivano commented Jan 23, 2023

Totally agree, let's merge it to dev (which will create pre-release packages anyway) and iterate from there.

@mivano mivano merged commit fa5b097 into serilog-contrib:dev Jan 23, 2023
@nenadvicentic nenadvicentic deleted the feature/automatically-handle-typename branch January 23, 2023 13:09
@mivano
Copy link
Contributor

mivano commented Jan 23, 2023

I updated the dotnet-version as well and changed the test to only run for net6 and net7. This caused the tests to complete. There are now new ci packages in the GitHub packages.

mivano added a commit that referenced this pull request Feb 2, 2023
* Create codeql-analysis.yml (#370)

* fix: Correct comment about default TypeName (#393)

The default TypeName was changed to '_doc' in #298

* Proper handling of TypeName = null from appsettings.json (#420)

Co-authored-by: Marius Wingerei <mawi@berg-hansen.no>

* Add `ElasticsearchSinkOptions.BufferFileRollingInterval` option (#416)

* Add `ElasticsearchSinkOptions.BufferFileRollingInterval` option

- Using this option we can customize buffer file rolling interval. The default is `RollingInterval.Day` (so no changes here). In some cases higher granularity may be needed.
- Changed regular expression for FileSet to get buffer files to support different rolling interval file name formats - from Infinite to Minute. All of them are different amount of digits representing date - 0(Infinite), 4(Year),..., 12 (Minute). So replaced expression part for day format `(?<date>\\d{8})` with the expression for all interval date `(?<date>\\d{0,12})`.

* Add tests for the desired functionality, which fail now.

- Return code to support only Daily rolling interval
- Add RollingIntervalExtensions.cs (origin: Serilog.Sinks.File) with InternalVisible attribute to be able to test
- Add InternalVisible to FileSet.cs to be able to test it
- Sign tests assembly the same way as Serilog.Sinks.Elasticsearch for InternalVisible to work

* Support different rolling intervals for DurableElasticsearchSink rolling files.

- Make support only for intervals like Day, Hour, Minute. As for less frequent intervals we cannot get specific date (specific day) for passing to _getIndexForEvent in `ElasticsearchPayloadReader`.
- Support handling rolling files for different intervals in `ElasticsearchPayloadReader` and `FileSet` by using corresponding formats and search patterns.
- Add tests of changed code - for `ElasticsearchPayloadReader` and `FileSet`

* Remove redundant spaces

* Fix internal or IntelliSense typos (#406)

Cleaning up a few very minor typos in internal methods or exposed via
IntelliSense:

 * "semi column" -> "semi-colon"
 * "preforming" -> "performing"
 * "CreatePlayLoad" -> "CreatePayload"

Co-authored-by: Bo Flynn <Bo.Flynn@nethealth.com>

* Clean package sources

* clean obsolete .net versions

* updated packages

* added dependabot

* Bump actions/setup-dotnet from 1 to 2 (#431)

Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 1 to 2.
- [Release notes](https://github.com/actions/setup-dotnet/releases)
- [Commits](actions/setup-dotnet@v1...v2)

---
updated-dependencies:
- dependency-name: actions/setup-dotnet
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump actions/checkout from 2 to 3 (#427)

Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 3.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2...v3)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump actions/cache from 2 to 3 (#429)

Bumps [actions/cache](https://github.com/actions/cache) from 2 to 3.
- [Release notes](https://github.com/actions/cache/releases)
- [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md)
- [Commits](actions/cache@v2...v3)

---
updated-dependencies:
- dependency-name: actions/cache
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump actions/download-artifact from 2 to 3 (#424)

Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 2 to 3.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v2...v3)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* added dependencies

* copy local files

* updating versions

* publish test results

* remove old nuspec and updated icons

* added icon

* updates

* fix build

* multiple packages

* name

* cicd

* skip test for now as it is flaky

* version

* Names

* use correct folder

* older version

* diffferent way of pushing

* enviroment

* Remove app veyor and dotnet-version

* Use repo owner

* use dotnet nuget

* logging

* remove @

* --skip-duplicate

* update project url

* Updated changelog

* paths

* use correct output path

* remove the source

* skip duplicates

* Bump actions/setup-dotnet from 2 to 3.0.2 (#478)

Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 2 to 3.0.2.
- [Release notes](https://github.com/actions/setup-dotnet/releases)
- [Commits](actions/setup-dotnet@v2...v3.0.2)

---
updated-dependencies:
- dependency-name: actions/setup-dotnet
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update cicd.yaml

* Bump actions/download-artifact from 1 to 3 (#439)

Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 1 to 3.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v1...v3)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump actions/upload-artifact from 2 to 3 (#438)

Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 2 to 3.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@v2...v3)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update cicd.yaml

* fix file references in the Visual Studio Solution file. (#461)

Co-authored-by: Nenad Vicentic <vicentic@immounited.com>

* Bump actions/setup-dotnet from 3.0.2 to 3.0.3 (#487)

Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 3.0.2 to 3.0.3.
- [Release notes](https://github.com/actions/setup-dotnet/releases)
- [Commits](actions/setup-dotnet@v3.0.2...v3.0.3)

---
updated-dependencies:
- dependency-name: actions/setup-dotnet
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump github/codeql-action from 1 to 2 (#426)

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 1 to 2.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@v1...v2)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Automatically handle `TypeName` parameter for different versions of Elasticsearch (<7, 7 or higher) (#462)

* fix file references in the Visual Studio Solution file.

* Do not set `TypeName` by default any more.
* last version of Elasticsearch that supported user-defined `_type` field - v6.8.x is running out of support on 2022-02-10 (https://www.elastic.co/support/eol)

* Automatically handle `ElasticsearchSinkOptions.TypeName` for different versions Elasticsearch (<7, 7+) when `ElasticsearchSinkOptions.DetectElasticsearchVersion` is enabled.

* Add unit test for automatic settings of `TypeName` when `DetectElasticsearchVersion` is set to `true`.
- Two methods used - instantiate `ElasticsearchSinkState` directly and via `LoggerConfiguration`

* Add Elasticsearch v8 template + parsing of Elasticsearch major version to `int` + decision branch for which version of index-template API to use + removal of obsolete `ElasticsearchTemplateProvider.GetTemplate(...)` method overload.

* Upgrade to .NET 6.0 and update test-frameworks related NuGet pacakges

* Upgrade to Elasticsearch.NET 7.17.5 + handle new "preflight" request

https://discuss.elastic.co/t/the-client-is-unable-to-verify-that-the-server-is-elasticsearch-due-to-an-unsuccessful-product-check-call-some-functionality-may-not-be-compatible-if-the-server-is-running-an-unsupported-product/310969/9

"The 7.16 client performs a pre-flight GET request to the root URL of the server before the first request.".

* Make `ConnectionStub` a bit more robust .

* Use `System.Version` to parse Elasticsearch server version number (similar to what `Elasticsearch.Net` does)

* Update NuGet packages

* Replace obsolete NuGet package `Serilog.Sinks.ColoredConsole` with `Serilog.Sinks.Console`

* Update `Serilog.Sinks.PeriodicBatching` package and reimplent `ElasticsearchSink` so that it does not use obsolete `PeriodicBatchingSink` constructor.

* Better handling of Elasticsearch server version number in mocked `ConnectionStub`

* Cleanup: refactor to use single `JsonEquals` method.

* Turn on `DetectElasticSearchVersion` option by default. Assume version 7 on fallback.

* Cleanup: remove unused namespaces

* Cleanup: move `ElasticsearchSinkTestsBase` into `Stubs` subfolder.

* Refactor: extract `ConnectionStub` into a separate file.

* Fix: json comparison in .NET Framework 4.6+

* Run unit-tests on multiple .NET frameworks.

* Cleanup: remove unused NUnit runner package.

* Use newer, built-in compilation constants.
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives#conditional-compilation

* Use standard MSBuild property `IsPackable` for clarity.

* Cleanup: remove unused package refrence.

* Update GitHub actions

* docs: updated documentation to reflect  changes in behavior of the sink.

Co-authored-by: Nenad Vicentic <vicentic@immounited.com>

* Update dotnet sdk

* disable net48 tests

* set api key

* Remove support for Elasticsearch v2 and v5. (#488)

* Remove support for Elasticsearch v2 and v5.

* Code-conventions: add rule for underscore `_` on private fields (as it already is in the code).

* Remove GitHub's `set-output` command deprication warning.

Co-authored-by: Nenad Vicentic <vicentic@immounited.com>

* added path for tests

* fix: Example in README is incorrect #402 (#496)

* Applying right versioning

* Update AssemblyInfo.cs

* Remove `AssemblyInfo.cs` and move attributes to the `*.csproj` file. This enables setting assembly version via command line and CI. (#501)

1. Build and pack:
    dotnet build -c Release -p:Version=9.0.0-beta11
    dotnet pack -c Release --no-build  -p:Version=9.0.0-beta11
2. Pack (with implicit build)
    dotnet pack -c Release -p:Version=9.0.0-beta11

Co-authored-by: Nenad Vicentic <vicentic@immounited.com>

* Read Elasticsearch server version from a root page response (#502)

Co-authored-by: Nenad Vicentic <vicentic@immounited.com>

* Versioning and permission for unit tests

* [no ci]

* #498 Disable `PublicSign` to fix strong-name signature verification issue of assemblies from public NuGet package: (#504)

> Could not load file or assembly 'Serilog.Formatting.Elasticsearch' or one of its dependencies. Strong name signature could not be verified. The assembly may have been tampered with, or it was delay signed but not fully signed with the correct private key. (Exception from HRESULT: 0x80131045)"

Co-authored-by: Nenad Vicentic <vicentic@immounited.com>

* [no ci]

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Mikkel Nylander Bundgaard <mikkelbu@users.noreply.github.com>
Co-authored-by: mariwing <mariwing@gmail.com>
Co-authored-by: Marius Wingerei <mawi@berg-hansen.no>
Co-authored-by: Andrey Kozlov <andrey.kozlov@skuvault.com>
Co-authored-by: Bo Flynn <boflynn@gmail.com>
Co-authored-by: Bo Flynn <Bo.Flynn@nethealth.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Nenad Vićentić <nenadvicentic@users.noreply.github.com>
Co-authored-by: Nenad Vicentic <vicentic@immounited.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants