search

exceptionless / Exceptionless.Net

Exceptionless clients for the .NET platform
https://exceptionless.com
Other
555 stars 142 forks source link

Add project to serialize/deserialize events and settings with MessagePack #168

Closed edwardmeng closed 6 years ago

edwardmeng commented 6 years ago

Improve the serialization/deserialization performance of the object storage with MessagePack, please refer to #163.

edwardmeng commented 6 years ago

@niemyjski I have finished unit tests, samples and bug fix, pls review the changes.

niemyjski commented 6 years ago

Thanks for the updates, we are very grateful and the latest changes look good. Talking to @ejsmith and @srijken we think we should move the equalities, get hash code changes out to a different pr so we can discus this and work on that separately and keep this PR focused on message pack and get it merged in quickly.

edwardmeng commented 6 years ago

The changes about equality compare methods(Equals and GetHashCode) have been reverted, and added another comparer only for unit test purpose, please refer to https://github.com/exceptionless/Exceptionless.Net/pull/168/commits/55d33eaac623383d47a19753a8a54a3c75fcbf7d. I agree with that we should create new pr to talk about the changes.

edwardmeng commented 6 years ago

The Exceptionless.MessagePack.Tests project has been changed to support netcoreapp2.0 and net462, and fixed bug of event deserialization and unit test running issues in appveyor.

edwardmeng commented 6 years ago

The equality compare relative methods have been reverted as your suggestion, and the unit tests for message pack serializer are skipped. The unit tests for duplication checker have been removed too. Waiting for you to merge it, and another pr will be requested.

niemyjski commented 6 years ago

Thanks a million for the help, pr and reverting the equality changes (Helps keep it focused and easier to test / get in). Your help is greatly appreciated. Do you need us to push a release out there soon for this or do you want the equality changes in with the next release?

From looking on your tests and talking with you and @srijken there looks like there are some equality issues that need to be addressed. Here are our concerns (we should open a new pr and or issue to discus but I just wanted to put them out there :).

  1. We need more benchmarks of previous GetHashCode implementation + equals implementation on how it effects the plugin pipeline / duplicate checker (we have a few duplicate checker plugins). This is an extremely hot code path for everyone and we need to ensure we do not make it slower at all costs.
  2. I know that we cannot add Count, Date (there was another one) event properties to GetHashCode() as those can change from event to event and those properties to us don't determine equality. On the previous comment, we've determined overtime that LineNumber and LineColumn cannot be included. These two values can and will change during runtime and will break deduplication. Basically the JIT can change the offset as well as the state of symbols (if I remember correctly).

On a side note, we made you a contributor a week ago :). Please feel free to join us on slack.exceptionless.com :)

edwardmeng commented 6 years ago

I recommend the next release to include the equality changes and duplicate checker improvements. The following days I will busy again, and have no time to request pr. I am so sorry for that.

zhouyongtao commented 6 years ago

It's looks like a pretty cool