First stab at taking this puppy to 2.0 #1
Conversation
…ce with the existing sink, introduced state object for common state values such as client, serializer to be constructed in one place
| template = this._templateMatchString, | ||
| settings = new Dictionary<string, string> | ||
| { | ||
| {"index.refresh_interval", "5s"} |
There was a problem hiding this comment.
Do we also need to specify something regarding the number of shards/replicas? What is the best practice for that?
There was a problem hiding this comment.
Compression is on by default now?
There was a problem hiding this comment.
best practice for shards/replica's is the default, we could open it up so folks can set it though.
compression is on by default since atleast 0.90.
There was a problem hiding this comment.
If you really want to fine tune it, people will most likely fire up their own mapping template anyway. As long as we can enable/disable this option I would not bother.
|
Nice progress! I quickly scanned through the code and will do some more testing later. |
|
Hi @Mpdreamz , what do we need to do to bring this live? Something I can do? |
|
@Mpdreamz Please let me know how to proceed with this? Want me to do testing, pick it up etc? thanks |
|
Hey @mivano sorry for the delay was out for 8 day long conference + company meetup when you posted this first. Totally slipped my mind. This needs reviewing/testing and merging :) !! |
|
No problems, heard the nice announcements regarding Elasic. Congrats with the new organisation.I will pick up the testing part. |
|
First quick test; Will do more testing soon. |
|
Tested what I could test and it all looks fine! Yes we do not react yet on the bulk index operation, but the stack overflow issue and the exception serialization issue are already a big win here. 👍 I merged everything to master but not yet pushed to github to avoid the CI. Questions is what to do about versioning? There are breaking changes and this should actually become a v2 package. @nblumhardt / @Mpdreamz any ideas? Having the 1.4.x link is nice for the compatibility between serilog and this sink, but not for the compatibility between the different sink versions. Are we willing to push 1.4.x+1 and provide more documentation for the people running into issues or do we want to make this 2.0.x? |
|
Thanks for validating @mivano ! Is CI set up already and if so does it matter e.g does it automatically push to nuget? We can create tickets for the other two issues, acting on bulk item failures and we should get a build script going (if its not there already). I am strongly in favor of dubbing this puppy I also want to spend a couple of hours to fully validate the current template one last time. I'll be sure to do that this week so this doesn't linger |
|
Also did you get the durability mode to work correctly? Otherwise we'd need to get that fixed before releasing 2.0. |
|
The CI will kick in when I push master and will generate the nuget packages. I just pushed master as a dev branch to this repository instead so we can continue from that one. |
|
Durability works now. Issue was with the serialization as it was not outputting the lines with a newline. |
|
BTW, it does no longer compile for .NET 4.0 due to the use of IReadOnlyDictionary (https://msdn.microsoft.com/en-us/library/hh136548%28v=vs.110%29.aspx) in the ElasticsearchJsonFormatter. Any non .NET 4.5 code possible here? |
|
I changed that to IDictionary instead. Also fails on https://msdn.microsoft.com/en-us/library/system.reflection.assemblyname.culturename(v=vs.110).aspx but fixed that with a conditional if. |
|
Something else to consider; the project builds 4.5 code, references 4.5 Nuget packages, but the nuspec file finds the files from the net\4.0 folder <file src="bin40\Release\Serilog.Sinks.Elasticsearch.dll" target="lib\net40" />
<file src="bin40\Release\Serilog.Sinks.Elasticsearch.xml" target="lib\net40" />@nblumhardt What is the game plan for targeting both 4.0 and 4.5? I would assume that we build twice and add the outputs for 4.0 and 4.5 inside the same nuget package or not? And do we still want to support 4.0? |
|
@mivano the build script should already handle this; it just builds 4.0 first, then 4.5, then packs the NUSPEC. You need to be careful not to (Not sure if you spotted it - @Mpdreamz 2.0 definitely sounds like the way to go 👍 Let me know if you want more of a hand - have been grappling with the 4.0/4.5 monster for a while so should be able to sort this out pretty quickly (unless the ES libs no longer target 4.0?) |
|
I noticed indeed some issue with the packages. I thought I corrected it in dev, but not entirely sure. If you could have a look, please. I think @Mpdreamz wants to do a check on the mapping, but after that we can push for the 2.0 version. I take it that you need to set that in appveyor? |
|
@mivano CI is already set up, and building packages (.NET 4 + 4.5) from https://ci.appveyor.com/project/serilog/serilog-sinks-elasticsearch AIl that's missing on the CI side is to switch on NuGet publishing. Will take a look at |
|
Thanks! @Mpdreamz can you let me know when you are satisfied with it? Then we can deploy this one. I started with some documentation already. |
|
Sweet 👍 documentation! I'm going to spend 2 hours tomorrow morning reviewing and doing a final sanity check myself. |
|
Nice, do you think we can also include the ability in the mapping to force the fields.1, fields.2 etc to be strings always? Now I have that when the first time it becomes an integer and the next time it is a string, the mapping prohibits the data from saving. By always forcing these indexed (?) properties to be strings we are safer. |
|
Hey @mivano evaluated the dev branch today for a final sanity check and made the following changes:
Other then that it looks good to me, whatever is left (log on self log) we can push in minor updates :) |
|
Cool! The ShouldRegisterTheCorrectTemplateOnRegistration tests fails currently. |
|
And I do not know what to do with that .NET 4 issue: |
|
Fixed the failing test also noticed the Hopefully that makes AppVeyor happy :) |
|
Great news! Let's merge to master and get this live. Want me to merge or can you do this? — On Wed, Apr 1, 2015 at 8:51 PM, Martijn Laarman [email protected]
|
|
I won't be able to for the next hour so you do the honors :) |
|
It is alive: http://www.nuget.org/packages/Serilog.Sinks.ElasticSearch/2.0.20 |
|
Aww yeah! Somebody tweet this! :) @mivano your work on the wiki rocks! Just noticed it.
|
|
Congrats guys, love your work! :) |
See serilog/serilog#394 for more information on the work needing to be done.
This PR represents the work that I have done so far (originally on a branch of the main repos).
I also remerged serilog/serilog#391 (the current master has various unresolved conflicts) and made sure it plays nice with the proposed changes for serilog/serilog#394.
AutoRegisterTemplateto options will cause the sink to try and register the template, the template takesIndexFormatinto account to know what to match on (see unit tests)ExceptionMethodobject allowing you to filter on exception location (https://gist.github.com/Mpdreamz/9fe1e18aca522f50b702)ElasticSearchextension methods and replaced all occurrences of the wrong casing, including the nuget ID, which might mean we have to nudge the nuget folks when releasing.new()clients and formatters and such uniformly by introducing anstateobject.NOT DONE: