api: Consider enabling (or having a feature for) tonic's "compression" feature

[barakmich]

What problem are you trying to solve?

Compiling console-subscriber into a package that already has some protos and tonic compiled in.

If these protos are using tonic's compression feature, they will enable a couple of fields (accept_compression_encodings, send_compression_encodings) that appear in console-api's generated protobufs that currently expect a unit.

How should the problem be solved?

Regenerate console-api's protos with tonic's compression turned on (it ought to be backward compatible if it's on)

Any alternatives you've considered?

Allow a feature, either through console-subscriber through to console-api to optionally enable the compression feature

How would users interact with this feature?

No response

Would you like to work on this feature?

maybe

[hawkw]

Hmm. I'm not sure if we should forward all of tonic's feature flags...instead, an application can just add its own tonic dependency and enable that feature flag, and it will be enabled for console-api as well.

I agree that the "compression" feature is often useful and should typically be enabled, but if we start adding feature flags to enable specific tonic features, the question is...where do we stop? In general, I think it only makes sense for a crate to have feature flags for a feature of a dependency when additional code in that crate is required to support the dependency's feature. Otherwise, it makes more sense for users who need a feature enabled on a transitive dependency to enable those crates features via their own dependency.

On the other hand, the console-subscriber crate maybe should enable the "compression" feature by default, because it actually just spins up an entire server on its own...and tokio-console, the client, should definitely enable "compression", so that it can talk to servers which enable compression. But, I'm unconvinced that it makes sense for the console-api crate to also have a feature for enabling "compression" — any crate that depends on console-api will generally have its own tonic dependency, and can just enable whatever tonic feature flags it wants to directly.

[barakmich]

That's a completely reasonable argument, and I agree that it should really be up to the application's choice of tonic feature flags.

But therein lies the problem -- right now the application can't choose to use the compression flag, because then console-api fails to compile (its generated protos not expecting the fields to be other than ())

Specifically, errors like:

Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/console-api-0.2.0/src/generated/rs.tokio.console.instrument.rs:338:33
    |
338 | ...                   accept_compression_encodings,
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `EnabledCompressionEncodings`, found `()`

error[E0308]: mismatched types
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/console-api-0.2.0/src/generated/rs.tokio.console.instrument.rs:339:33
    |
339 | ...                   send_compression_encodings,
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `EnabledCompressionEncodings`, found `()`

error[E0308]: mismatched types

when the application has compression turned on

I'd be more than happy if the application can choose 👍

[hawkw]

But therein lies the problem -- right now the application can't choose to use the compression flag, because then console-api fails to compile (its generated protos not expecting the fields to be other than ())

Ah, hmm...so it seems like the issue here is that we've checked in the generated Rust code for the protos, rather than generating them every time the crate is built. This was changed in #318 in order to remove the build-time dependency on protoc every time console-api is built.

However, it seems like an unfortunate consequence of that change is that now, the compression feature (and presumably, any other feature flags which affect the generated Rust code are now no longer additive, and enabling a feature flag causes build failures. This is not kosher for how cargo feature flags are expected to work. I think this might mean that we should stop checking in generated Rust code, and instead switch back to generating the Rust code at build-time. cc @LucioFranco, what do you think?

[barakmich]

Yeah, exactly! Ironically I found this issue because I wanted to generate our own protos and remove the build-time dependency. Generating them turned on compression, and, here we are.

I'm good with any solution, but yeah, non-kosher feature flags are rough.

[neoeinstein]

I think I may have an angle that can allow us to remove the compression flag from tonic-build. This would allow for checking in the generated code while still leaving the decision whether to include the compression machinery to the end application (via the compression flag on tonic).

[neoeinstein]

I just opened a PR on tonic that, if accepted, should allow generated code to be agnostic to the eventual compression features enabled while building an application.

[caibirdme]

Hi, is there any solution for this? How could I compile if I rely on compression flag

[hawkw]

Hi, is there any solution for this? How could I compile if I rely on compression flag

It looks like @neoeinstein's upstream PR hyperium/tonic#1004 has merged, but hasn't been released (as it's a breaking change). When tonic 0.8 is released, that should resolve this issue.

I think the upstream fix is almost certainly the best solution for this issue. In the meantime, I would consider accepting a PR that changes the console-api build process to not check the generated code in to the repo, and builds it in a build script instead (the way we did prior to #318. That way, a downstream dependent enabling the tonic-build/compression feature would result in console-api generating code that supports compression.

This would re-introduce the build-time dependency on protoc for compiling crates that depend on console-api, which is somewhat unfortunate. We should check with the Tonic maintainers to see if a release with hyperium/tonic#1004 will happen in the near future --- if it's going to happen soon, it would be preferable to not have to do a workaround that re-introduces the build-time protoc dep.

[nashley]

I am no longer experiencing this issue now that tonic 0.8 has been released (using the gzip feature in tonic and no added features in tonic-build). Is there anything left to track in this issue?

[hawkw]

We need to update our tonic dependency, I believe. But, it seems that the problem has, in fact, been fixed upstream.

[LucioFranco]

Think we can close this, right? :D