feat: clean up lint.sh, use API to set grpc layer #467

TommyCpp · 2021-03-04T02:32:43Z

As per Remove tracer provider guard. #444 (comment).
We should try to throw compile errors when users set two exclusive features instead of default to use one of them.
Added a precommit.sh to run fmt, clippy and test
Clean up the lint.sh a little bit. Haven't worked with Bash for a while so please let me know if something doesn't look right.

djc · 2021-03-04T10:36:28Z

IMO making features non-additive is frowned upon in the ecosystem. Perhaps we could do it as part of the tests to flush out issues in our test suite, but I don't think we should intentionally start doing this for our published libraries.

Cargo.toml

frigus02 · 2021-03-04T17:28:24Z

IMO making features non-additive is frowned upon in the ecosystem. Perhaps we could do it as part of the tests to flush out issues in our test suite, but I don't think we should intentionally start doing this for our published libraries.

I'm not super happy with this, either. If I understand correctly, the features are already mutually exclusive, though. At the moment we just silently ignore one of the features if the other one is enabled. I would prefer a helpful error message instead.

That said, I'm still undecided if it would be better to configure these things at runtime.

The scripts look fine to me. I think a trailing newline might be nice. I think that's common for shell scripts, too.

TommyCpp · 2021-03-04T20:52:16Z

I think the real problems I would like to solve is how do we tell user that featureA and featureB is exclusive and should only enable one of them. Some of our crates have complex features. For example, in opentelemetry-otlp we have following features.

async = ["default"]
trace = ["opentelemetry/trace"]
metrics = ["opentelemetry/metrics"]
default = ["tonic", "tonic-build", "prost", "tokio"]
grpc-sys = ["grpcio", "protobuf", "protobuf-codegen", "protoc-grpcio"]
tls = ["tonic/tls"]
tls-roots = ["tls", "tonic/tls-roots"]
openssl = ["grpcio/openssl"]
openssl-vendored = ["grpcio/openssl-vendored"]
integration-testing = ["tonic", "tonic-build", "prost", "tokio/full", "opentelemetry/trace"]

This basically comes down to four choices.

Is it trace or metrics.
If it's trace, for grpc layer, is it tonic or grpcio.
Is it tls/ssl or not encrypted
Different choice on how to encrypted.

But I don't think it's trivial to understand how to choose between those features without some knowledge of the implementation.

djc · 2021-03-04T21:32:16Z

trace or metrics doesn't have to be an either/or choice, or does it?

I made a change in hyper-rustls which removed the compiler error on feature conflict in favor of having the API be explicit about which feature to prefer. rustls/hyper-rustls@fc60b8c I think this is probably the best way to do it if possible, though I don't quite understand why the grpc vs tonic features (for example) necessarily/naturally conflict.

TommyCpp · 2021-03-05T01:49:51Z

trace or metrics doesn't have to be an either/or choice, or does it?

Sorry those two are not mutually exclusive. 🤦‍♂️

Thanks for the example! I think it could be a good approach.

though I don't quite understand why the grpc vs tonic features (for example) necessarily/naturally conflict.

I think it's mostly because we have different ways to build the client based on different grpc layers. But I will try to poke around to see if we can use the API to separate those two features.

TommyCpp · 2021-03-06T16:58:01Z

Still need to add docs, change tests and fix small nits. But my general idea for this crate is to use an enum to represent different grpc layers.

The reason for not using trait is that the trace exporter here has two tasks,

convert the otel spans into otlp format
send it via grpc.

In our current implementation, tonic and grpcio take different approaches in both of those tasks. It's hard to abstract the behavior as it involves a lot of sturcts like Spans, Links, etc.

If users want to bring their own grpc layer, they can just implement a new tracer exporter with SpanExporter trait and install it.

… same time. As per open-telemetry#444 (comment). We should try to throw compile error when users set two exclusive features instead of default to use one of them.

The TracerExporter basically have two tasks. The first one is convert OTEL trace to binary, the second one is send it. `tonic` and `grpcio` take different approaches in both of those two tasks. Thus, we use enum here to represent two different grpc layers. If user want to bring their own grpc layer. They can just implement a new TracerExporter with SpanExporter trait and install it.

frigus02 · 2021-03-09T18:49:32Z

I'm still on the fence between cargo features and setting things up at runtime. Since cargo doesn't have a way of describing that features are mutually exclusive, I'm leaning slightly towards the setup in Rust code at runtime.

This looks quite nice to me. Didn't see anything worth commenting on. 👍

Do you want to update docs/tests/etc in this PR?

djc

I think the API-based approach works out pretty well, I especially like the builder setup.

djc · 2021-03-09T20:52:46Z

opentelemetry-otlp/src/lib.rs

+#[cfg(
+not(feature = "integration-testing")
+)]


This looks a little odd?

I think the idea is to hide the proto mod unless we are running integrationt testing.

djc · 2021-03-09T20:53:51Z

opentelemetry-otlp/src/lib.rs

-    not(feature = "integration-testing")
-))]
-#[allow(clippy::all, unreachable_pub, dead_code)]
+#[allow(warnings)]


Why are we allowing all the warnings here? Maybe add a comment about this?

Mostly because the proto mod only contains generated code. We should change it.

djc · 2021-03-09T20:57:09Z

scripts/lint.sh

+  cargo_feature opentelemetry-jaeger "reqwest_collector_client"
+  cargo_feature opentelemetry-jaeger "collector_client"
+
+fi


Nit: add a newline?

jtescher

LGTM @djc is this good to merge or do you have additional feedback?

djc · 2021-03-15T10:17:16Z

LGTM. @TommyCpp, good work!

…nc feature is not set. This was introduced in open-telemetry#467. I believe the async feature was there to spin up a tokio runtime if the exporter was not already in one. It's mostly because the tonic cannot work without a tokio runtime. Renamed the feature to be less confusing. Also clean up all the clippy problems in metrics module and add some links into doc.

TommyCpp force-pushed the tommycpp/exclusive-features branch from 3bb9b97 to cef5eea Compare March 4, 2021 03:17

TommyCpp marked this pull request as ready for review March 4, 2021 03:17

TommyCpp requested a review from a team March 4, 2021 03:17

djc reviewed Mar 4, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

TommyCpp marked this pull request as draft March 6, 2021 16:47

TommyCpp force-pushed the tommycpp/exclusive-features branch 3 times, most recently from 9a350b6 to b2a5540 Compare March 6, 2021 21:33

TommyCpp changed the title ~~feat: clean up lint.sh, make sure exclusive features don't set at the same time~~ feat: clean up lint.sh, use API to set grpc layer Mar 6, 2021

djc mentioned this pull request Mar 8, 2021

Prepare for v0.13.0 release #470

Merged

TommyCpp added 3 commits March 8, 2021 19:30

feat: clean up lint.sh, make sure exclusive features don't set at the…

09bec4d

… same time. As per open-telemetry#444 (comment). We should try to throw compile error when users set two exclusive features instead of default to use one of them.

merge upstream

685c26b

TommyCpp force-pushed the tommycpp/exclusive-features branch from b2a5540 to 685c26b Compare March 9, 2021 00:33

TommyCpp marked this pull request as ready for review March 9, 2021 03:07

djc reviewed Mar 9, 2021

View reviewed changes

fix: address comments.

a262e40

TommyCpp requested a review from djc March 11, 2021 23:52

jtescher approved these changes Mar 13, 2021

View reviewed changes

Merge branch 'main' into tommycpp/exclusive-features

829f289

djc approved these changes Mar 15, 2021

View reviewed changes

Merge branch 'main' into tommycpp/exclusive-features

8c2e313

jtescher merged commit e9ef3e2 into open-telemetry:main Mar 15, 2021

TommyCpp mentioned this pull request Mar 17, 2021

fix: otel-otlp will try to create another tokio runtime inside one if async feature was not enabled. #483

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: clean up lint.sh, use API to set grpc layer #467

feat: clean up lint.sh, use API to set grpc layer #467

TommyCpp commented Mar 4, 2021 •

edited

Loading

djc commented Mar 4, 2021

frigus02 commented Mar 4, 2021

TommyCpp commented Mar 4, 2021

djc commented Mar 4, 2021

TommyCpp commented Mar 5, 2021 •

edited

Loading

TommyCpp commented Mar 6, 2021

frigus02 commented Mar 9, 2021 •

edited

Loading

djc left a comment

djc Mar 9, 2021

TommyCpp Mar 10, 2021

djc Mar 9, 2021

TommyCpp Mar 10, 2021

djc Mar 9, 2021

jtescher left a comment

djc commented Mar 15, 2021

feat: clean up lint.sh, use API to set grpc layer #467

feat: clean up lint.sh, use API to set grpc layer #467

Conversation

TommyCpp commented Mar 4, 2021 • edited Loading

djc commented Mar 4, 2021

frigus02 commented Mar 4, 2021

TommyCpp commented Mar 4, 2021

djc commented Mar 4, 2021

TommyCpp commented Mar 5, 2021 • edited Loading

TommyCpp commented Mar 6, 2021

frigus02 commented Mar 9, 2021 • edited Loading

djc left a comment

Choose a reason for hiding this comment

djc Mar 9, 2021

Choose a reason for hiding this comment

TommyCpp Mar 10, 2021

Choose a reason for hiding this comment

djc Mar 9, 2021

Choose a reason for hiding this comment

TommyCpp Mar 10, 2021

Choose a reason for hiding this comment

djc Mar 9, 2021

Choose a reason for hiding this comment

jtescher left a comment

Choose a reason for hiding this comment

djc commented Mar 15, 2021

TommyCpp commented Mar 4, 2021 •

edited

Loading

TommyCpp commented Mar 5, 2021 •

edited

Loading

frigus02 commented Mar 9, 2021 •

edited

Loading