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 Getter.Keys() with Jaeger Baggage support. #1549

Merged
merged 15 commits into from
Nov 5, 2020

Conversation

carlosalberto
Copy link
Contributor

@carlosalberto carlosalberto commented Aug 18, 2020

Fixes #967

Implements the open-telemetry/opentelemetry-specification#825 update, and implements Jaeger Baggage support to verify that the API is correct/useful (based on their own propagator)

  • Used Iterable for keys().
  • Had to remove the usage of anonymous classes in the integration test's trace-context section, as it broke some versions of Java 11 (see this).
  • We can play with removing the (most likely) slow regex in a follow-up.
  • Don't put an empty Baggage in Context in case there was not Baggage to be extracted at all.

For Jaeger reviewers:

  • Didn't encode the Baggage values. In Jaeger there's two TextMap propagators registered by default (encoded and non-encoded propagation). We need to define the encoding case in a follow-up (somehow).

@open-telemetry/java-approvers
cc @pavolloffay @jpkrohling

@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@28905b7). Click here to learn what that means.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1549   +/-   ##
=========================================
  Coverage          ?   84.30%           
  Complexity        ?     1819           
=========================================
  Files             ?      215           
  Lines             ?     7354           
  Branches          ?      800           
=========================================
  Hits              ?     6200           
  Misses            ?      852           
  Partials          ?      302           
Impacted Files Coverage Δ Complexity Δ
.../io/opentelemetry/opentracingshim/Propagation.java 95.83% <0.00%> (ø) 5.00 <0.00> (?)
.../extension/trace/propagation/JaegerPropagator.java 90.74% <100.00%> (ø) 41.00 <13.00> (?)
...lemetry/sdk/extension/otproto/TraceProtoUtils.java 76.19% <0.00%> (ø) 6.00% <0.00%> (?%)
...n/java/io/opentelemetry/api/common/Attributes.java 92.00% <0.00%> (ø) 18.00% <0.00%> (?%)
.../metrics/aggregator/DoubleLastValueAggregator.java 100.00% <0.00%> (ø) 7.00% <0.00%> (?%)
.../io/opentelemetry/sdk/trace/NoopSpanProcessor.java 100.00% <0.00%> (ø) 8.00% <0.00%> (?%)
...io/opentelemetry/sdk/trace/data/ImmutableLink.java 100.00% <0.00%> (ø) 5.00% <0.00%> (?%)
...lemetry/context/PersistentHashArrayMappedTrie.java 45.45% <0.00%> (ø) 4.00% <0.00%> (?%)
...opagation/B3PropagatorInjectorMultipleHeaders.java 100.00% <0.00%> (ø) 4.00% <0.00%> (?%)
.../opentelemetry/sdk/metrics/InstrumentRegistry.java 100.00% <0.00%> (ø) 6.00% <0.00%> (?%)
... and 207 more

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 28905b7...1999c72. Read the comment docs.

@richardstartin
Copy link

The problem with making this a collection is it will often need to be materialised, even if you can get away with calling Map.keySet in a lot of cases. Making it an Iterable would be lighter-weight in most cases, e.g. Kafka client, old enumeration based context carriers (e.g. JMS, HttpServletRequest)

@carlosalberto
Copy link
Contributor Author

I'd be fine to have keys() return an Iterable or Iterator.

@jkwatson
Copy link
Contributor

Can you add the concrete example of how the Jaeger propagator will use this new feature to this PR? I think that will be very useful for the spec PR, as well, to demonstrate how you're doing to use it.

@carlosalberto carlosalberto marked this pull request as ready for review October 26, 2020 13:40
@carlosalberto carlosalberto changed the title Initial addition of Getter.keys(). Add Getter.Keys() with Jaeger Baggage support. Oct 26, 2020
* Returns all the keys in the given carrier.
*
* @param carrier carrier of propagation fields, such as an http request.
* @since 0.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we've gotten rid of all the @since tags for now; we'll set them all to 1.0 when we're ready to release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I totally forgot about it. Will do.

return builder == null ? null : builder.build();
}

@SuppressWarnings("StringSplitter")
Copy link
Contributor

Choose a reason for hiding this comment

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

not relevant for this PR in particular, but I think we should turn off this warning globally, since it's telling us to use guava, which we don't to do for the API, and won't, ever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

@Test
void extract_Nothing() {
// Context remains untouched.
assertThat(
jaegerPropagator.extract(
Context.current(), Collections.<String, String>emptyMap(), Map::get))
Context.current(), Collections.<String, String>emptyMap(), getter))
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a little safer/cleaner to just use Context.root() here, so we know that we don't have a pre-polluted context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, will update.

@jkwatson
Copy link
Contributor

This looks ok (although it makes me sad that our Getter is no longer a SMI). Based on the discussion above, I had thought you would go with an Iterable/Iterator, rather Collection as the return type for keys(). Did something change your mind?

@carlosalberto
Copy link
Contributor Author

I had thought you would go with an Iterable/Iterator, rather Collection as the return type for keys(). Did something change your mind?

Wanted to give a last try at Collection and to verify it's still fine ;) But if you think we should try enumerable instead (as mentioned in the previous related conversation), then I will go with enumerable for sure.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

The Jaeger specific parts are looking good to me, but I'm not an expert in the propagation format. Perhaps @objectiser or @yurishkuro would want to double-check as well?

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Mostly looks nice thanks


for (String key : getter.keys(carrier)) {
if (key.startsWith(BAGGAGE_PREFIX)) {
if (key.length() <= BAGGAGE_PREFIX.length()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we already checked startsWith I think this can be equals check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather stick to check the length, as it only depends on a numerical comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I meant an equal check on the length - if it starts with it, then the length is guarateed to be at least that length and the < here looks redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@yurishkuro
Copy link
Member

In Jaeger there's two TextMap propagators registered by default (encoded and non-encoded propagation). Which one is the default?

Neither is default. In OT, HTTP and TextMap are different, so Jaeger url-encodes baggage for HTTP, but not for TextMap (eg in Kafka headers there's no need to url-encode).

The jaeger-baggage header support only happens at extraction, not injection, which makes me believe it's deprecated? Should we mention this in the docs?

That is correct. The official baggage format (in and out) is uberctx-{key}. jaeger-baggage header was invented later for manual injection via curl.

@carlosalberto
Copy link
Contributor Author

Neither is default. In OT, HTTP and TextMap are different, so Jaeger url-encodes baggage for HTTP, but not for TextMap (eg in Kafka headers there's no need to url-encode).

Oh, interesting. @yurishkuro , what do you think we should do here? Maybe have two versions, one that does encoding, one that does not? Or maybe something else? e.g.

  private JaegerPropagator(bolean doBaggageEncoding) {
     this.doBaggageEncoding = doBaggageEncoding; // Depending on this, baggage will be encoded or not.
  }

@carlosalberto
Copy link
Contributor Author

@open-telemetry/java-approvers Please review the latest updates.

@yurishkuro Updated the code based on your feedback. In a follow up we can add the encoded-values case, if that works for you.

@carlosalberto
Copy link
Contributor Author

@yurishkuro Also, there's the question on whether, upon no existing baggage at all in the carrier, we should put an empty Baggage in Context (for example, if SpanContext was extracted but no baggage was detected at alll).

@yurishkuro
Copy link
Member

upon no existing baggage at all in the carrier, we should put an empty Baggage in Context

I would try to avoid that. Even if there is an immutable singleton Baggage.empty(), adding it into an immutable context still requires re-allocation of the context, which is a malloc for little benefit. If would just make Baggage.fromContext(ctx) return empty singleton when key is not present.

@jkwatson
Copy link
Contributor

jkwatson commented Nov 4, 2020

looks like this needs a rebase, @carlosalberto

@carlosalberto
Copy link
Contributor Author

@jkwatson Rebased ;)

@carlosalberto carlosalberto merged commit 5195622 into open-telemetry:master Nov 5, 2020
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.

Add Jaeger propagator
6 participants