Replace play-json with nanojson, leave play-json tests for now.

[dvgica]

This is a follow-up from #888. The PR removes the dependency on play-json and instead writes JSON using https://github.com/mmastrac/nanojson, which is already required by the kamon-status-page module.

Since play-json is widely used in the Scala community, there's potential for dependency conflicts when a user pulls in both kamon-datadog and their own play-json. This PR avoids that situation.

A few questions for the maintainers:

• Should we shade the nanojson dep, like kamon-status-page does?
• I left the tests as they are, using play-json. Should we put in the work to port them to nanojson? I haven't yet for a few reasons:
  • to make sure the JSON output was exactly the same while changing the underlying implementation,
  • play-json has some niceties when working in Scala that nanojson does not, so replacing it there would be a bit annoying (and I'm lazy :-D )
  • play-json can be moved to a test dep so conflicts will no longer be an issue

[SimunKaracic]

Good call on first changing only the code
It would be nice to refactor the tests too, but I'll let it slide if you insist on being lazy :D

Should we shade the nanojson dep, like kamon-status-page does?

I think we should, but I just published this locally, and I don't see nanojson included in the jar ¯\(ツ)/¯

All in all, great PR, thanks for the contrib! 🎉

[SimunKaracic]

LGTM, I'll merge tomorrow when the tests finish running, and someone else checks out the sbt changes

[dvgica]

Laziness aside, the two JSON libs have different requirements for the different contexts. In the actual code, we want something fast, reliable, small, and non-conflicting. In the tests, we really only care about correctness and ease of use - a difficult-to-use JSON lib will discourage test-writing or make the tests difficult to understand. So, I think there's an actual case for leaving play-json in that role. It's also nice to use a different JSON lib in tests because it ensures compatibility (i.e. it checks that nanojson isn't doing something non-compliant).

[dvgica]

LGTM, I'll merge tomorrow when the tests finish running, and someone else checks out the sbt changes

Awesome, thanks for the quick turnaround and feedback on this!

[SimunKaracic]

Laziness aside, the two JSON libs have different requirements for the different contexts. In the actual code, we want something fast, reliable, small, and non-conflicting. In the tests, we really only care about correctness and ease of use - a difficult-to-use JSON lib will discourage test-writing or make the tests difficult to understand. So, I think there's an actual case for leaving play-json in that role. It's also nice to have to use a different JSON lib in tests because it ensures compatibility (i.e. it checks that nanojson isn't doing something non-compliant).

Well when you put it that way, I agree that it's better to leave tests as is!

[dvgica]

I think we should, but I just published this locally, and I don't see nanojson included in the jar ¯_(ツ)_/¯

When I published kamon-datadog locally with the shading changes, I do see it included in the JAR in a shaded fashion (under /kamon/lib).

[SimunKaracic]

I think we should, but I just published this locally, and I don't see nanojson included in the jar ¯_(ツ)_/¯

When I published kamon-datadog locally with the shading changes, I do see it included in the JAR in a shaded fashion (under /kamon/lib).

Ok, we're almost there, please just remove the shading part, it's not necessary, and we can merge 🎉

[dvgica]

Ok, we're almost there, please just remove the shading part, it's not necessary, and we can merge

👍 done!

[SimunKaracic]

Done!

Oh, and for the future, you don't have to push an empty commit to rerun tests, the reviewer can (and should) do that for you