feat: Add Monix and ZIO bundles, Micrometer and PureConfig interop

[jakubjanecek]

No description provided.

[hanny24]

So the plan is to have a single micrometer module that will implement metrics for various other modules?

[jakubjanecek]

It doesn't necessarily must be like that but yes. This can be seen in the pureconfig module (package implicits) where ConfigReaders for other modules are implemented.

[augi]

Maybe it would be better to start with README, so we could see your intents earlier?

[jakubjanecek]

@sideeffffect I've updated the code according to your comments.

@augi Yes, I guess I need to do that though I wanted to avoid that in case you would shred it to pieces :) If I could describe it in few sentences the point is that we have module per dependency (e.g. http4s-blaze-server and pureconfig) and these modules sometimes need to interoperate. My original idea was an idea of "bundle" which is a module that depends on both these dependencies and ties them together - we could have as many as we wanted of those with different combinations and with some duplication of code (though the duplication should be quite limited). Another option would be to create lots of small modules with different dependency combinations and only use them in "bundles" to share some code (to avoid bit of the duplication but with lots of extra work for us). And than you offered me the solution with optional dependencies which I don't like because of possible runtime dependencies (and I cannot really always use the trick with failing user's code by forcing him to provide one of the classes from the dependency) but in the end I think it offers me the best of both worlds which is that I keep implementation of interop code close the the main dependency (e.g. implementation of ConfigReader from PureConfig for all other modules inside the pureconfig module but with only optional dependency on other modules) but I force the user to make an explicit import (with proper import name) to be aware that something special is used (if you're importing com.avast.sst.pureconfig.implicits.Http4sBlazeClient._ you should probably realize that you need to have sst-http4s-blaze-client dependency in your project). I also presume that "bundles" which will also be used to bundle certain combinations of dependencies should prevent most of runtime exceptions because they will already bundle together necessary dependencies.

[jakubjanecek]

I'll work on this tomorrow to add some documentation and examples. I'll make it a proper full-blown PR which you can evaluate.

[hanny24]

So my take on this is to avoid optional dependencies and make many small modules instead. First of all, I believe that creating a new module is fairly easy and does not really take much time.
Secondly, I think that many small modules is a good practice as we can see in many scala libraries: for instance take a look here or here.

And lastly, this solution is quite scaleable as you should only integrate only two libraries together.

[jakubjanecek]

I kind of agree because that was my original intent and the most "pure" solution. However the situation is a bit different with this library. What you refer to is a single library interoperating with N external libraries which results in N SBT modules. What we have is M SBT submodules interoperating with N external libraries which results in theory in M times N SBT submodules. Right now we have PureConfig interoperating with 5 other libraries so we would have to create 5 other submodules. For example we'd have to create two submodules for Micrometer - one for JMX and one for StatsD implementation. Now imagine that in a year we'll replace or add (more probable) new configuration library "FutureConfig"... you have to double the number of submodules immediately. And it gets worse, we now support single implementation of http4s but I can very easily imagine adding support for different implementations, e.g. supporting HTTP client implemented using Apache. So yeah, creating a submodule is easy but having tens of modules with usually only single file seems quite extreme. Thoughts?

[hanny24]

I'm afraid I don't see how Micrometer is related configuration library. Say I have http4s. I would expect following modules:

• sst-http4s
• sst-http4s-blaze
• sst-http4s-apache
• sst-http4s-micrometer
• sst-http4s-pureconfig

For me this is very straight forward, clean and manageable. My point is that optional dependencies is just an excuse for our laziness that will hurt us in the future. BTW we already tried something like this before with typeclasses library that we have and it did not worked and we had to split it.

[jakubjanecek]

Can you maybe try to explain more what was the problem you encountered?

I get what you propose and again I kind of agree. But we do have http4s-blaze-server and http4s-blaze-client so you'd have to double these and the way Micrometer and PureConfig are "related" is that we need to configure Micrometer and we do that by some case class and we need to load that case class from Lightbend Config. Look at the code, it's all there: https://github.com/avast/scala-server-toolkit/pull/30/files#diff-c23502bc80a3b79260a553e543fd78a9

Also I think that you have the naming in reverse. sst-http4s-pureconfig somehow evokes that it's http4s' implementation for PureConfig whereas what we have is PureConfig's implementation for http4s and thus it should be sst-pureconfig-http4s-blaze at least (the same it works here: https://pureconfig.github.io/docs/library-integrations.html).

So this is what we would need now (if I am not forgetting something):

• sst-http4s-blaze-server
• sst-http4s-blaze-client
• sst-pureconfig
• sst-pureconfig-http4s-blaze-server
• sst-pureconfig-http4s-blaze-client
• sst-micrometer
• sst-micrometer-jmx
• sst-pureconfig-micrometer-jmx
• sst-micrometer-http4s-blaze-server
• sst-micrometer-http4s-blaze-client

And just by adding new Micrometer implementation (StatsD) and possibly new http4s implementation (Apache) you can immediately add like another 8 or maybe 10 submodules. To me it seems like it's not scalable at all and will get out of hand quite quickly.

The problem is that our library does not provide anything new, it's just a mix of other libraries so dependencies might be the biggest problem we're dealing with. That's why I also think that copying others is not the best argument.

[jendakol]

I'm not sure about my opinion TBH. As a librarian I went through miscellaneous paths in solving problem like this (even though it was never potentially so big as it is now) so I know how all those solutions suck.
On the other hand, we have to choose some, I'm afraid. So here is my opinion on proposed:

1. N times M modules - on will go crazy creating and maintaining (!) huge number of modules and I'm not really sure "users" will really appreciate it
2. modules (micrometer, pureconfig, ...) with optional dependencies - this could work, but there's big but. Once we want to avoid NoClassDeffFoundError and similar beauties, it's necessary to have the module written in a way user cannot make a mistake. It means, something like this is very bad/sad: "Do not forget to have a dependency on the sst-micrometer-jmx module in your project" (or it will explode in runtime).
   I don't know right now how to solve it, but the idea is to have these things used only in case the user has the right dependency. I have an example - if you consider having an implicit val readerServer: Reader[Server] where Server is something from optional dependency, and it will be used only implicitly, it's kind of clear that compiler will use this val only when Server is really present on classpath (because why would it require an instance of some typeclass for it otherwise?). It's not 100% anyway, but IMHO better then the state proposed in current PR.

[jakubjanecek]

Jenda points out an important fact. If I make all the PureConfig reader definitions implicit lazy val it should never happen that NoClassDefFound is thrown because unless you have the type on the classpath you will never try to get the implicit definition for it, you would have to explicitly reference it which would be very strange.

Micrometer module is a bit different but I think I do what Augi told me on Slack which is that I require some class from the dependency which again prevents you from the runtime exception because your code wouldn't compile.

Please consider that because I think it makes the optional dependencies more digestible ;)

[hanny24]

Can you maybe try to explain more what was the problem you encountered?

• Runtime errors as it was not very clear when things get accessed/initialized.
• It wasn't very user friendly as it was difficult to track where is what implemented (eg. if configuration via pureconfig is implemented as optional dependency on pureconfig in http4s module, or optional dependency on http4s in pureconfig module)

I get what you propose and again I kind of agree. But we do have http4s-blaze-server and http4s-blaze-client so you'd have to double these and the way Micrometer and PureConfig are "related" is that we need to configure Micrometer and we do that by some case class and we need to load that case class from Lightbend Config. Look at the code, it's all there: https://github.com/avast/scala-server-toolkit/pull/30/files#diff-c23502bc80a3b79260a553e543fd78a9

Could you please be more specific what I'm supposed to look at? I still don't see why would you need two modules for http4s and micrometer integration. Just define metrics trait in a module (or reuse MetricsOps in case of http4s) and provide micrometer implementation in a separate module.

Also I think that you have the naming in reverse. sst-http4s-pureconfig somehow evokes that it's http4s' implementation for PureConfig whereas what we have is PureConfig's implementation for http4s and thus it should be sst-pureconfig-http4s-blaze at least (the same it works here: https://pureconfig.github.io/docs/library-integrations.html).

Well, that's subjective:) But I don't really care about this as long as it's consistent.

So this is what we would need now (if I am not forgetting something):

* `sst-http4s-blaze-server`
* `sst-http4s-blaze-client`
* `sst-pureconfig`
* `sst-pureconfig-http4s-blaze-server`
* `sst-pureconfig-http4s-blaze-client`
* `sst-micrometer`
* `sst-micrometer-jmx`
* `sst-pureconfig-micrometer-jmx`
* `sst-micrometer-http4s-blaze-server`
* `sst-micrometer-http4s-blaze-client`

And just by adding new Micrometer implementation (StatsD) and possibly new http4s implementation (Apache) you can immediately add like another 8 or maybe 10 submodules. To me it seems like it's not scalable at all and will get out of hand quite quickly.

But then we won't really fulfill one of our goals: to have small building blocks. We will end up with a few big ones. And if I take into extreme, we can have single module only.

[jveselka]

Also I think that you have the naming in reverse. sst-http4s-pureconfig somehow evokes that it's http4s' implementation for PureConfig whereas what we have is PureConfig's implementation for http4s and thus it should be sst-pureconfig-http4s-blaze at least (the same it works here: https://pureconfig.github.io/docs/library-integrations.html).

I don't think that would be very practical. You usually ask yourself "How can I improve my sst-http4s-blaze-server with additional modules?" (by looking for modules with prefix sst-http4s-blaze-server). Asking yourself "What more can I configure with sst-pureconfig" would be strange.

[jakubjanecek]

@hanny24 I have to think about your message so I'll respond later.

@jveselka Get it but that leads directly to the "excessive number of submodules" state so in a way the reverse naming (though probably less natural) could work better.

Let me ask you this. Is there anyone who thinks optional dependencies are a viable/better/simpler/whatever solution?

(it seems like Jenda thinks so but if everyone else is against it I guess there's no point in discussing this more)

We could also try to help us and make less granular submodules. For example I wonder whether http4s-server and http4s-client need to be split because usually you want both. I also split the jvm-* modules into multiple ones (asked by @jveselka) but since they have the same dependency which is JVM I wonder whether we could put them back together to help ourselves a bit. I am afraid of getting unnecessary dependencies into my project, not unnecessary small pieces of code (such as SSL context configuration which I usually don't use at all). What do you say?

[hanny24]

I'm ok with merge modules as long as all of them require same set of dependencies. It the merged module turns out to be big, we can always split it later.

[tomasherman]

So for me if the goal is to have unopinionated library for composing small pieces together into a backend, multiple small modules is the way to. Now we should discuss whether unopinionated is really a way to go.

The tradeoffs are:

Optional dependencies

• 👍 less work upfront when creating sst and user application
• 👎 possibility of runtime errors (and even nasty ones in cases when some class is not needed until hours after deployment and it gets tricky to even detect that something bad is going on)

Modules

• 👍 more work upfront
• 👎 more headache at runtime

Neither of those choices is very appealing. Which makes me question whether having unopinionated toolbox is a way to go.

Btw https://git.int.avast.com/skyline/backend-common can be used as example of what library with many small modules looks like - it's up to you to decide whether it's worth it to have it structured this way (and our approach is much more opinionated that this library is going to be).

[sideeffffect]

my 2 haléře:

• if we wanted to be failing in run-time, we could be already using Python, so if the work is not absurdly demanding, I would be for having more smaller modules
• agree with @jveselka , I like the "added fature" as the last thing in the name, so sst-http4s-blaze-client-pureconfig, or sst-http4s-blaze-server-micrometer.
• maybe the names should be even sst-http4s-client-blaze-pureconfig, or sst-http4s-server-blaze-micrometer, because there are other http4s servers and clients besides the ones based on blaze

[jakubjanecek]

Thank you everyone for your comments. I've decided to get rid of optional dependencies and reworked it into multiple smaller modules (on the other hand I merged all the jvm-* modules because it was unnecessary to have them split). I still need to work on some documentation and examples and maybe tests but please have a look at it whether it now makes sense to you.

[hanny24]

Could you please split it into multiple smaller PRs? It's way to big.

[jakubjanecek]

Sorry but I cannot spend more time on redoing this. I know there's a lot of changes and I will do better next time but as it's all inter-related it would be quite difficult to commit the changes one-by-one and not spend another day or two on this. Please bear with me and try to have a look at it.

The most important is whether the artifact names, package names and the way implicits are imported (PureConfig) makes sense to you. Other changes are usually just cosmetic or moving of files from the past. I also added Micrometer monitoring but you've also seen that in the past and last is two bundles for Monix and ZIO which just add Server trait (just a helper) and bundle dependencies together.

[jakubjanecek]

I've added some tests, comments, documentation and most importantly fixed the build (caused by case-sensitivity of the file system on Travis). This PR is ready to be merged from my PoV.

[sideeffffect]

object with implicits should be called implicits, no?

[sideeffffect]

so where do you @jakubjanecek want that green thumb exactly?