Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thoughts about compatibility with ZIO and also tracing #500

Open
valenterry opened this issue Aug 5, 2024 · 10 comments
Open

Thoughts about compatibility with ZIO and also tracing #500

valenterry opened this issue Aug 5, 2024 · 10 comments

Comments

@valenterry
Copy link

The ZIO ecosystem is awesome and very pragmatic. However, when it comes to ZIO and ZQuery, the lack of abstraction shows a bit.

ZQuery is missing quite a few convenience functions that are on ZIO (even though it should be no problem to add them on ZQuery). Also, there is no common interface, so when having code that needs to work with both sometimes, common abstractions are harder.

Furthermore, there is no OpenTelemetry support for ZQuery unless I missed something - that is really a bummer.

Thoughts?

@kyri-petrou
Copy link
Collaborator

Hey @valenterry, thanks for the feedback!

ZQuery is missing quite a few convenience functions that are on ZIO

I agree, although given how big the ZIO API is, it'll require a fair bit of work to support all of its methods. Are there any specific ones you feel like you're often missing? Also, feel free to open a PR to add any of them as you see fit!

Also, there is no common interface, so when having code that needs to work with both sometimes, common abstractions are harder.

I'm a little bit confused regarding this one. Can you provide an example of such usecase?

Furthermore, there is no OpenTelemetry support for ZQuery

I agree that it would be nice for Datasources to be instrumented for OTEL. However, I'm not sure to what degree this is useful since zio-query uses the ZIO runtime, which most OTEL libraries already support. In addition, this needs to be implemented at the OTEL libraries, not in zio-query. Which OTEL library do you use? Perhaps we could add an OTEL module for ZQuery in zio-opentracing.

@valenterry
Copy link
Author

I'm a little bit confused regarding this one. Can you provide an example of such usecase?

For example, I use caliban. Some of my endpoints are defined as ZIO and some are defined as ZQuery. I have some functions factored out that first run common logic (e.g. auth and logging) and then run the effect. Their signature is something like def runInContext[...](f: Context => ZIO[...]) which means, I need to duplicate those functions for ZIO and ZQuery, even if I use the same functionality (like flatMap only).

As for the OpenTelemetry part, I'm actually not 100% sure how it work conceptually, given that ZQuery is precisely there to abstract over the querying-flow. As for what I use, that would be GCP's opentelemetry package.

@kyri-petrou
Copy link
Collaborator

kyri-petrou commented Aug 5, 2024

Lifting a ZIO to ZQuery can be done via the ZQuery.fromZIO and ZQuery.unwrap methods, and ZIO effects can be "flatMapped" with ZQueries via the mapZIO method.

I think this is as best as we can do since ZQuery is a more powerful variant of ZIO, therefore we can only lift them one-way (from ZIO to ZQuery)

@valenterry
Copy link
Author

Yeah, but if I have either a ZIO or ZQuery at hand, then I need to either convert them (which is annoying) or use runInContextZIO or runInContextZQuery depending on the value I have. I could overload runInContextZIO but overloading comes with its own disadvantages.

Since both can be me e.g. mapped, it would theoretically be possible to have a common trait Mappable (or Functor/Applicative ...) to use as an interface to avoid that.

However, I doubt that this will ever happen in the ZIO ecosystem - please just consider it a little rant. :)

As for the missing functionality, I will add some of these (I hope). Like ZQuery.filter(...) for example.

@kyri-petrou
Copy link
Collaborator

kyri-petrou commented Aug 6, 2024

Since both can be me e.g. mapped, it would theoretically be possible to have a common trait Mappable (or Functor/Applicative ...)

I believe what you're asking for can be achieved by using the zio-prelude library. However, it only provides type classes for ZIO. Potentially we could also add type classes for ZQuery, but that would require a bit of effort and publishing a new package (zio-query-prelude?). I think it will be a good addition, but I got my hands a bit full at the moment so I'm not sure when I'd be able to get to it

@valenterry
Copy link
Author

I believe what you're asking for can be achieved by using the zio-prelude library.

Oh yeah, I forgot that this exists. This is exactly what should be used IMHO.
There is e.g. AssociativeFlatten which we could add a ZQuery instance for. But since ZQuery does not seem to depend on ZIO directly, yeah, it should be in a separate package I guess.

If you could setup the build/project structure then I'm happy to add the instances in there.

@sideeffffect
Copy link
Member

Perhaps zio-query can start depending on zio-prelude. Then, zio-query can implement the relevant type classes for its own types in its own codebase.

@paulpdaniels
Copy link
Collaborator

I don't know if we'd want to add dependency to core since that has downstream effects (for instance in caliban). I think either way we'd want a separate module, just a question of if it goes here or in zio-prelude

@sideeffffect
Copy link
Member

sideeffffect commented Sep 19, 2024

Definitely not in zio-prelude! The point of zio-prelude is that other libs in the ZIO family depend on it. Not the other way around.

I don't know if we'd want to add dependency to core since that has downstream effects

You already depend on ZIO itself 😄 ZIO Prelude is the second most "core" library, right after ZIO. If it will make the decision easier for you, we'll be publishing 1.0.0 of ZIO Prelude in the near future.

separate module

Orphan instances are pure pain. I always recommend avoiding it if at least a little bit possible.

@kyri-petrou
Copy link
Collaborator

I think it should be possible to make zio-prelude an optional dependency and provide type classes for it. This way users that do use zio-prelude with zio-query will have the type classes available. There is a pattern we use in caliban that makes it possible to do this without getting runtime errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants