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

Remove grpcio #153

Merged
merged 3 commits into from
Nov 23, 2021
Merged

Remove grpcio #153

merged 3 commits into from
Nov 23, 2021

Conversation

Geal
Copy link
Contributor

@Geal Geal commented Nov 17, 2021

Fix #150
supplant #147

This PR includes:

  • commits cherry picked from make the OTLP exporters non exclusive #147 and refactored, to allow multiple OTLP exporters to be used in the same compiled binary
  • removing grpcio, in favor of using tonic for OTLP/gRPC
  • renaming the otlp-tonic feature to otlp-grpc
  • the tls feature forces otlp-grpc since it cannot be used independently (maybe we could merge them?)
  • allow choosing the http or grpc exporter in the configuration without setting any fields (7437492). I'm not sure about that one, I may be making some invalid yaml here

@Geal Geal force-pushed the remove-grpcio branch 3 times, most recently from 5d79e66 to 3a84c54 Compare November 19, 2021 10:24
@Geal Geal marked this pull request as ready for review November 19, 2021 13:54
@Geal Geal force-pushed the remove-grpcio branch 3 times, most recently from 7437492 to fafa071 Compare November 19, 2021 14:08
@netlify
Copy link

netlify bot commented Nov 19, 2021

❌ Deploy Preview for apollo-router-docs failed.

🔨 Explore the source changes: fafa071

🔍 Inspect the deploy log: https://app.netlify.com/sites/apollo-router-docs/deploys/6197afcc8015120007aa6ac3

crates/apollo-router/Cargo.toml Outdated Show resolved Hide resolved
crates/apollo-router/Cargo.toml Outdated Show resolved Hide resolved
crates/apollo-router/src/configuration/mod.rs Outdated Show resolved Hide resolved
crates/apollo-router/src/configuration/otlp/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, this looks good to me overall, I would however rather have us refer to Grpc* instead of Tonic*, both in the structures we define and the test files

crates/apollo-router/src/configuration/mod.rs Outdated Show resolved Hide resolved
crates/apollo-router/src/configuration/mod.rs Outdated Show resolved Hide resolved
@Geal
Copy link
Contributor Author

Geal commented Nov 19, 2021

tbh we could just remove the tls feature and have it always built with otlp-grpc. It increases the produced binary by only 2MB (1MB when stripped) and does not increase the requirements on the system: it load the same set of dlls as a router built without the tls feature

@o0Ignition0o
Copy link
Contributor

tbh we could just remove the tls feature and have it always built with otlp-grpc
This sounds good to me, I can hardly think of a case where we wouldn't wanna do this (for the binary that is).

@Geal
Copy link
Contributor Author

Geal commented Nov 22, 2021

so the License checker apparently did not test with all the features. @o0Ignition0o how should we add the license for ring since it's not well detected?
https://app.circleci.com/pipelines/github/apollographql/router/414/workflows/4a64d40e-fbd7-4693-8751-c7fd9c9741f6/jobs/1661/parallel-runs/0/steps/0-111
https://github.com/briansmith/ring/blob/main/LICENSE

@o0Ignition0o
Copy link
Contributor

@Geal i have done it here #164 so I’d rather merge this one instead:D

@o0Ignition0o
Copy link
Contributor

o0Ignition0o commented Nov 23, 2021

Ok this has just been merged, CI should pass once you rebase!

@Geal Geal force-pushed the remove-grpcio branch 2 times, most recently from e541826 to b77d0ad Compare November 23, 2021 09:47
Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre aproving this, There's still a mention of TonicExporter which IMO should be GrpcExporter or something. otherwise LGTM

#[serde(deny_unknown_fields)]
pub struct Exporter {
pub struct TonicExporter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GrpcExporter ?

We do not need to use two different gRPC implementations for OTLP, and
grpcio greatly increases the compilation times and CI cache size, so
we're choosing to use tonic instead

* make the OTLP exporters non exclusive

We should be able to build a router with both OTLP/HTTP and OTLP/gRPC
exporters

update the opentelemetry dependency to get access to
SpanExporterBuilder that was not public before
* The cargo feature name should match the actual feature, not the
implementation detail

* allow empty configuration under http or grpc exporter

with this we can configure the HTTP or gRPC exporter while using default
values, like:
opentelemetry:
  otlp:
    tracing:
      exporter:
        http:
@Geal Geal merged commit 3f28207 into main Nov 23, 2021
@Geal Geal deleted the remove-grpcio branch November 23, 2021 10:53
@Geal Geal self-assigned this Dec 1, 2021
tinnou pushed a commit to Netflix-Skunkworks/router that referenced this pull request Oct 16, 2023
…defer-in-introspection

support defer in introspection queries
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

Successfully merging this pull request may close these issues.

do we need 2 different GRPC implementations?
3 participants