opentelemetry: remove tonic dependency

[djc]

See #403. This just moves the code into an example -- I'm not sure what else would be relying on these impls?

[codecov]

Codecov Report

Merging #414 (5983185) into master (aae3f4e) will decrease coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
- Coverage   48.65%   48.41%   -0.24%     
==========================================
  Files          65       65              
  Lines        5523     5492      -31     
==========================================
- Hits         2687     2659      -28     
+ Misses       2836     2833       -3     

Impacted Files	Coverage Δ
opentelemetry/src/propagation/mod.rs	100.00% <ø> (+4.87%)	⬆️
...ntelemetry/src/sdk/metrics/aggregators/ddsketch.rs	76.76% <0.00%> (-0.20%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aae3f4e...5983185. Read the comment docs.

[jtescher]

Think this was it. Was added for integration convenience to avoid tonic users having to copy this into all their apps I believe, but likely better as external crate or just copy/paste as locking the version here is probably makes the whole thing not worth it.

[djc]

Good. Shall I get rid of http too, then?

[jtescher]

Sounds good 👍

[TommyCpp]

IMHO we still should provide the tonic integration as the external crates if we can so that it can be used out of the box. Usually, Users would likely want to spend less time on telemetry compared with business logic, which means they should be able to do it with less configuration.

[djc]

It's not clear how that would work with the current definition of the Extractor/Injector traits, though. (Also, I think those names could be improved -- they're more like Injectable and Extractable.)

[jtescher]

@djc yeah the names come from the propagators api spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#injectors-and-extractors-as-separate-interfaces, we don't need to have the names match exactly if we can add clarity. The rust naming convention for single method traits seems to be more aligned with the method name like ToString::to_string or Clone::clone, we currently follow that other places like ShouldSample::should_sample, could consider doing that for inject and extract.

[djc]

I think my point on naming is a little different and potentially orthogonal to what you're saying. The problem that had me a little confused at first is that IIRC Rust traits that are named for a verb can actually "do the thing" (so you can read() on Read for example), whereas with Injector and Extractor the injecting/extracting is done "to the thing" that implements the traits. This is where I thought it might make sense to change the interface, but ran aground on the notion of a global propagator (not sure exactly how/why that gets used): if we defined an Inject interface that takes a type param for the type it can inject into, we'd be able to solve the coherence issues.

[jtescher]

Yeah the global propagator is tricky to express. E.g. if an actix-web or rocket middlware crate wants to be able to extract headers for a given app, then it needs a way to find the globally configured propagator so it knows which headers to extract (b3, jaeger, w3c trace context, some combination, etc). Thus the current global method for getting a reference to an impl of the TextMapPropagator trait which can inject via a dyn Injector and extract via a dyn Extractor (which to your point may be easier to understand as Injectable and Extractable).

Having the global propagator generically accessible for instrumentation anywhere in the app seems like the challenge for the type system currently.