encoding/json: Add custom JSON package with build tag support for Sonic

[shazbert]

PR Description

This pull request introduces a custom JSON package located in encoding/json/ to facilitate swapping between the default Go encoding/json package and the sonic library based on build tags.

Adds:

• json.go (implementation using encoding/json).
• sonic.go (default implementation using sonic library).

Import Updates:

All imports to the standard encoding/json package have been updated to the new custom package github.com/thrasher-corp/gocryptotrader/encoding/json.

Added basic benchmarks for both the default encoding/json and the sonic implementation to evaluate hot-swap performance.

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How has this been tested

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.

• go test ./... -race
• golangci-lint run
• Test X

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally and on Github Actions with my changes
• Any dependent changes have been merged and published in downstream modules

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 37.07%. Comparing base (72a2a16) to head (135e2b6).

Files with missing lines	Patch %	Lines
cmd/exchange_wrapper_issues/main.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1623   +/-   ##
=======================================
  Coverage   37.07%   37.07%           
=======================================
  Files         414      414           
  Lines      180274   180274           
=======================================
+ Hits        66835    66845   +10     
- Misses     105574   105577    +3     
+ Partials     7865     7852   -13     

Files with missing lines	Coverage Δ
backtester/btcli/main.go	0.00% <ø> (ø)
backtester/config/backtesterconfig.go	88.46% <ø> (ø)
backtester/config/strategyconfig.go	83.51% <ø> (ø)
backtester/config/strategyconfigbuilder/main.go	0.00% <ø> (ø)
backtester/eventhandlers/statistics/statistics.go	74.63% <ø> (ø)
backtester/main.go	0.00% <ø> (ø)
cmd/apichecker/apicheck.go	38.20% <ø> (ø)
cmd/config/config.go	8.62% <ø> (ø)
cmd/config_builder/builder.go	0.00% <ø> (ø)
cmd/documentation/documentation.go	0.00% <ø> (ø)
... and 89 more

... and 11 files with indirect coverage changes

[shazbert]

Flagged this as medium because this is beast. Sorry for those that cant use it. 🙏

[shazbert]

Are you suggesting to default the main code to sonic out of the box not just the workflow tests?

Yes

Ok cool,

I propose that we're being too cautious, and creating unnecessary split code paths and feature support. I completely understand why it's reassuring to dip our toes in gently. But this either works right (and it probably does), or it doesn't.

I guess it comes from GCT library (as the more we update this library the further it drifts away from this and I could be completely wrong) wants to be as close to using internal dependencies as much as possible as default and all external dependencies are opt in. You're right in that this approach is being too cautious but I like that.

Also it had to do with sonics different configurations Default, Fastest, Standard(aligns most closely to encoding/json). I was thinking we could have different build tags for those in future if I or somebody else ever gets around to it (other things are way more priority). Also we could hot swap in different JSON libraries if they were similar if they provide a certain use case.

Anyways this is my perspective and I am not against sonic being default either. @thrasher- @gloriousCode for your input

[gbjk]

Looks good 👍 Great work 🎉

Only suggests again.

[gbjk]

Looks good.

If you take the suggestion to make sonic the default, then I'll re-review that, obviously.

[thrasher-]

This is a comment only about the overall ideaology

I see from the tests that we are defaulting to sonic off, and we're testing amd64 with sonic both on and off.

I propose that we're being too cautious, and creating unnecessary split code paths and feature support. I completely understand why it's reassuring to dip our toes in gently. But this either works right (and it probably does), or it doesn't.

I'm assuming our main production use cases are amd64/arm64 on linux, and we should optimise efficiency and ease of use for that main use-case. Guys, please correct that assumption and this proposal can die immediately. I'm assuming windows should be edge case for runtime/production.

Proposal / Endorsement:

• We should default to sonic on
• We should refuse to run with sonic off if it should be available according to the platform
• We then have only have edge case tests where we turn sonic off

We can default sonic to on, since we're already using another json package jsonparser by default in many exchanges and other systems now.
We can have one job with it disabled to catch potential issues (linux amd64).
The makefile can therefore be adjusted to support a NO_SONIC build param if people want to build with native JSON support instead.

[gbjk]

Great work 🎉

Only a couple of minor things.

[gbjk]

• Seems like we're using the nolint as the package comment across the 3 json/*.go files.
• encoding/json is ambiguous in the comment, since it's referring to golang.org/encoding/json but inside gct/encoding/json
• I don't think we need to say that all uses of json throughout should refer to it

So I think we want a single package comment (common.go?) and then package comments in the other two relevant to them, and then the depguard nolint comment just explains it:

// json is an abstraction middleware package to allow switching between json encoder/decoder implementations
// The default implementation is sonic.
// Build with `sonic_off` or `386` tags to switch to golang.org/encoding/json.

and

import "encoding/json" //nolint:depguard // Acceptable use in gct json wrapper

[shazbert]

done

[gbjk]

Super-Duper Nit: "Compilation" tastes weird
Compiling and Running

[gbjk]

Considered also splitting sections into Compiling and Running

[shazbert]

done sir

[gbjk]

Approved

[gloriousCode]

tACK

[thrasher-]

Just a note here:
Defaulting to this on may slow down our migration to 1.24 and future Go versions. If they take a week or so to migrate to the latest go major version then that is fine. If however, they take months then we'll switch to defaulting off as our general turn around time for PR for the past few releases is days as we like to take advantage of all the new Go features as soon as they come available. I've asked for a ETA on when they're going to merge so this will be a good test to see: bytedance/sonic#741