Closed toravir closed 6 years ago
Might be interesting.
@rs - just keeping you posted.. am more than half-way done with binary support - you could see the changes here: https://github.com/toravir/zerolog/commits/binary
I could use review comments...
I have a few questions:
cbor
package into event
/context
?json
package? I'm not against it but I don't think it belongs to this PR (and it will make the review harder).cbor
package custom made for this PR or copied/adapted from another project? If the later, we need to put copyright info.I did not review the cbor
package implementation yet. I need some time to read the spec etc. I will save that for when you are ready for the PR.
Good job!
@rs
Why CBOR instead of BSON? (This is a candid question, I don't know much about CBOR)
CBOR is more flexible with multiple different data types that BSON - bson is specific to how mongodb sends json objects across the wire. Plus CBOR is a RFC Standard. I had posted on JSON forum (json@yahoogroups.com) and got a recommendation to use CBOR. FYI - CBOR RFC: https://tools.ietf.org/html/rfc7049
How do you plan to plug the cbor package into event/context?
It is not as clean plug as i would have liked - but i have pushed one approach. Am open to other ways of doing this. The first cut is in this commit: https://github.com/toravir/zerolog/commit/a6948d48bd59d363ec48cafe12e9e448cf4f34d8#diff-9fb85d1ee9f1b0a59804bec41e1491fe
Why did you refactor the json package? I'm not against it but I don't think it belongs to this PR (and it will make the review harder).
Sure i can push that change as a different PR. The idea is to isolate all JSON specific writes to json package - so that common code does not have any JSON specific writes - only few minor changes.
Is cbor package custom made for this PR or copied/adapted from another project? If the later, we need to put copyright info.
I wrote this code from ground up. Other implementations i had seen were JSON to CBOR - could not be re-used for zerolog's purpose.
The global binary flag can be confusing. It gives the impression that it will turn all logging into binary, even existing instances. What it really does is changing the default encoding. Its name (and doc) should reflect that.
Sure will rename it to imply the correct meaning.
Thx
@toravir any news on that PR?
@rs - will submit PR shortly. Has two items in the todo - will commit within a week or two:
[Enhancement] having a binary logging support will be useful.. meaning keeping json formatted byte array means the code does a strconv for each data type. If we add an option to have binary log support, it will reduce the cpu and memory overhead. I did this simple test:
For Int type - current test passes '0' - it takes 31.2 ns/op
instead, if i put in MaxInt64 as field value, the time is 44ns/op: BenchmarkLogFieldType/Int-8 30000000 44.0 ns/op
I attribute it to strconv() taking longer time to generate all the digits.
The conversion of the binary log to a readable format can be done offline.
I did a small code snippet to do comparison of JSON/BSON - the eval code and results are available here: https://github.com/toravir/bson-eval
My proposal is that zerolog can be configured either to use JSON or BSON. All the BSON code will be added into internal/bson/ directory.
Please reply if you think this is useful addition to zerolog.