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

Using durable seq #205

Merged
merged 7 commits into from
Jan 26, 2019
Merged

Using durable seq #205

merged 7 commits into from
Jan 26, 2019

Conversation

pierresetteskog
Copy link
Contributor

@pierresetteskog pierresetteskog commented Dec 18, 2018

What issue does this PR address?
#179 Port buffer file improvements from Serilog.Sinks.Seq
#203 is fixed with BufferCleanPayload example can be found in Serilog.Sinks.Elasticsearch.Sample\Program.cs
#194 with BufferIndexDecider you can see the logrow and it is runned for every row see example program
#204 BufferFileCountLimit see example program
#166 SingleEventSizePostingLimit see example program
#146 fixed by using code from sink.seq

Does this PR introduce a breaking change?
No not what i know

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)
    almost no logical changes is made on the code just structural refactoring..
    Other information:
    code is mostly from sinks.Seq and placed in the folder Durable some classes like LogShipper PayloadReader is made generic with no dependencies to seq. Would be nice to place in some generic nuget package to share between different sinks.
    Lots of settings was just working in file mode or in memory mode.
    Maybe two different configure classes would be more clear.
    In the example program Program.SetupLoggerWithPersistantStorage
    I have tried to include all settings which is available in the persistant mode.
    Sorry for the big pull request but it was a big task to move over persistant code from sink.seq.

/// <summary>
///
/// </summary>
public class ElasticSearchLogShipper : LogShipper<List<string>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class ElasticSearchLogShipper : LogShipper<List<string>>
public class ElasticsearchLogShipper : LogShipper<List<string>>

Convention is to use Elasticsearch instead of ElasticSearch

/// <summary>
///
/// </summary>
public class ElasticSearchPayloadReader: APayloadReader<List<string>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public class ElasticSearchPayloadReader: APayloadReader<List<string>>
public class ElasticsearchPayloadReader: APayloadReader<List<string>>

@mivano
Copy link
Contributor

mivano commented Dec 27, 2018

Thanks for your PR! Looks good. I m not actively using this sink at the moment, so it is a bit hard for me to see if this will work out fine. Adding the Seq implementation is certainly a nice go forward and the other changes look sensible.
There are however some breaking changes; types are changed and method signature has some added, although default null, parameters. Not a big issues, but would be good to bump the major.

Can you also alter the changes.md and the readme.md with the relevant details? Then we can merge to dev and roll out a pre release package first.

thanks!

@pierresetteskog
Copy link
Contributor Author

Greate i will be on a vacation a week now will do it after that!

@pierresetteskog
Copy link
Contributor Author

I didnt know which version to use but you mentioned major version bump so i wrote 7. Please let me know if I should do something more. :)

@mivano mivano merged commit 4f3a7ab into serilog-contrib:dev Jan 26, 2019
@mivano
Copy link
Contributor

mivano commented Jan 26, 2019

Thanks @pierresetteskog. Merged to dev so we get packages. There is another PR in progress #208 that would be nice to include in a new release. Waiting for the completion of that one to final merge to master.

@mivano
Copy link
Contributor

mivano commented Jan 26, 2019

Package: https://www.nuget.org/packages/Serilog.Sinks.ElasticSearch/6.5.0-alpha0045

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.

2 participants