Get rid of Elasticsearch.Net dependency for Serilog.Formatting.Elasticsearch nuget package

[vhatsura]

At the moment, there is Elasticsearch.Net dependency in Serilog.Formatting.Elasticsearch nuget package. From Elasticsearch.Net is used only IElasticsearchSerializer interface. It's not a big issue due to Serilog.Sinks.Elasticsearch uses it for communication with ES, however, in case of using inly formatter without sink, it would be good to have dependencies as less as possible.

To remove Elasticsearch.Net dependency from Serilog.Formatting.Elasticsearch it can be specified custom Serializer interface in Serilog.Formatting.Elasticsearch project and write a simple adapter in Serilog.Sinks.Elasticsearch between IElasticsearchSerializer and custom Serializer interface. In such case,public contracts of ES Sink will not be changed.

What do you think about it?

[mivano]

I do agree that removing the Elasticsearch.Net reference does make the formatter more reusable. As far as I can quickly see, we only use it here directly: https://github.com/serilog/serilog-sinks-elasticsearch/blob/dev/src/Serilog.Formatting.Elasticsearch/ElasticsearchJsonFormatter.cs#L307

And that, in turn, calls the Serialize function in the interface using the method extensions and returns the byte array as a UTF8 string.
Looking at the default implementation of the Serialize function (https://github.com/elastic/elasticsearch-net/blob/67fe8ce6cbaea8ab593dfe77d038fa5b6bb4f903/src/Nest/CommonAbstractions/SerializationBehavior/InternalSerializer.cs#L69) it does not do very complicated things.

So it might be as easy as replacing the call with some other code instead of still having the dependency although via an adapter?

[mivano]

I see that you raised a PR that indeed removed the reference. However, is this adapter needed, since the actual usage of the Elasticsearch functionality is minimal? And what to do when you only need the formatter itself, how do you provide the serialization functionality?

[vhatsura]

So, I added adapter, because Elastichsearch sink accept IElasticsearchSerializer as a parameter and can be configured by library consumers. As I never use Elasticsearch sink and need only formatter, I don't know how often custom IElasticsearchSerializer is acctually used. Saving backward compatibility for Elastichsearch sink was a main reason to have adapter.
If we can introduce breaking changes and nobody need to specify custom serialization I'll delete adapter and update PR.
@mivano, what are your thoughts about it?

[mivano]

The more lightweight we can make it, the better. So if the usage of ES in this formatter is only minimal and we can change that by some other code instead, I would say we go for that.

[vhatsura]

So, it means that you suggest to remove ISerializer interface and remove possibility to define implementation IElasticsearchSerializer during ES sink configuration, am I right?

[mivano]

I might misunderstand the issue or your question, sorry for that, but my impression is that the formatter should not need a reference to ES at all since the code that actually uses it is pretty minimal. So remove the usage altogether and replace it with a direct call to a JSON text writer instead. Then there should be no need to have custom implementations. The actual sink itself uses the formatter regardless of the actual new implementation of the serializer in the formatter.
Is this also what you have in mind?

[vhatsura]

@mivano, just to clarify, which JSON text writer you mean? Serilog.Formatting.Elasticsearch references only Serilog and don't have any JSON text writer which can write object as json string.

[mivano]

Ah I see now. Yes, you are right; there is that serializer call to write the object to json which it gets indeed from Elasticsearch. It makes sense to make that abstract so you can pass in Newtonsoft or Elasticsearch, or in the future the one that is referenced in issue elastic/elasticsearch-net#3656.

Sorry for the confusion on my part, I think your original PR was a correct one.

[vhatsura]

No problem, I recreated initial PR. Can you review it, again?

Also, I found out interesting thing: file under src/Serilog.Sinks.Elasticsearch/Sinks/ElasticSearch/Durable/ElasticSearch is duplicated. Is it expected?

[mivano]

Looks good! Yes that file is strange. The correct name should be with Elasticsearch. So lowercase second s.