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

Add support for gzip compression to otel via tonic #1688

Open
darach opened this issue May 24, 2022 · 3 comments
Open

Add support for gzip compression to otel via tonic #1688

darach opened this issue May 24, 2022 · 3 comments
Labels
_complexity:low A task with a low complexity that should be easy to understand connector Connectors documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers performance performance enhancements or issues _size:small A small task that should be quick to do

Comments

@darach
Copy link
Member

darach commented May 24, 2022

Describe the problem you are trying to solve

Add support for configurable compression to OpenTelemetry connectors in tremor.

Describe the solution you'd like

Minimally supports configurable gzip compression and decompression of otel payloads
in conformance with the gRPC over HTTP/2 requirements.

Notes

hyperium/tonic#282

hyperium/tonic#692

Requested by @ekarlso

@darach darach added enhancement New feature or request documentation Improvements or additions to documentation good first issue Good for newcomers performance performance enhancements or issues _complexity:low A task with a low complexity that should be easy to understand _size:small A small task that should be quick to do connector Connectors labels May 24, 2022
@dak-x
Copy link
Contributor

dak-x commented Jul 2, 2022

I believe we would want to provide a new optional field (say "compression") in the configuration for the "otel" connectors.
Note that following the discussion in hyperium/tonic#692 compression is triggered for each and every payload, meaning no per request compression.

Updated Config:

Client

pub(crate) struct Config {
    /// The hostname or IP address for the remote OpenTelemetry collector endpoint
    #[serde(default = "Default::default")]
    pub(crate) url: Url<OtelDefaults>,
    #[serde(default = "default_true")]
    pub(crate) logs: bool,
    /// Enables the trace service
    #[serde(default = "default_true")]
    pub(crate) trace: bool,
    /// Enables the metrics service
    #[serde(default = "default_true")]
    pub(crate) metrics: bool,
    
    // Do we require `Option` for backwards compatibility?
   pub(crate) compression: Option<CompressionEnum> 

}

similarly,

Server

pub(crate) struct Config {
    /// The hostname or IP address for the remote OpenTelemetry collector endpoint
    #[serde(default = "Default::default")]
    pub(crate) url: Url<OtelDefaults>,
    #[serde(default = "default_true")]
    pub(crate) logs: bool,
    /// Enables the trace service
    #[serde(default = "default_true")]
    pub(crate) trace: bool,
    /// Enables the metrics service
    #[serde(default = "default_true")]
    pub(crate) metrics: bool,

   // Do we require `Option` for backwards compatibility?
   pub(crate) compression: Option<CompressionEnum> 
}

Compression Enum

pub Enum CompressionEnum {
    gzip,
   // ... add others as requested
}

@Licenser
Copy link
Member

Licenser commented Jul 2, 2022

Heya, this looks good to me :). There is no need for the Option if you implement Default for CompressionEnum and then tag the filed with #[serde(default = "Default::default")] - that way, when missing it's set to the default and the dance around checking if it's set or not is avoided.

@dak-x
Copy link
Contributor

dak-x commented Jul 2, 2022

Have you evaluated upgrading tonic from 0.6.2 to 0.7.2? If not I would evaluate this, as 0.6.2 seems to only support gzip explicitly with the
<Client>.send_gzip() and <Client.accept_gzip() methods on TonicClent.

While the latest iteration has a slightly different method signature i.e.
<Client.send_compressed(Compression::gzip/..) and Client.accept_compressed(Compression::gzip/) refer here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
_complexity:low A task with a low complexity that should be easy to understand connector Connectors documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers performance performance enhancements or issues _size:small A small task that should be quick to do
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants