fix!: refactor propagators to add #fields #638

fbogsany · 2021-03-05T02:49:37Z

Fixes #613

This is a large refactor of propagators. Part of the goal is to add the #fields method, however this also uncovered quite a few bugs, discrepancies, incorrect documentation, and deviation from the spec.

Important changes:

Added documentation for propagators and improved consistency & accuracy of existing docs
Replaced TextMapInjectors and TextMapExtractors with unified TextMapPropagators
Clarified that the carrier passed to #inject is expected to be mutable - inject now returns nil instead of the modified carrier - the spec states that "Carriers used at Inject are expected to be mutable." (although there is some later inconsistency)
Implemented #fields for each TextMapPropagator
Clarified references to TextMapPropagator rather than Propagator - the latter is an abstract concept in the spec, with only the former currently defined - the spec makes it clear that "A binary Propagator type will be added in the future"
Provided 3 implementations of TextMapPropagator in the API: a wrapper for a TextMapInjector and a TextMapExtractor pair, a CompositeTextMapPropagator that composes either a list of propagators or a list of TextMapInjectors and a list of TextMapExtractor`s, and a default no-op text map propagator

TODO

cleanup the tests
consider whether and how to enforce the spec's requirement that "In order to increase compatibility, the key/value pairs MUST only consist of US-ASCII characters that make up valid HTTP header fields as per RFC 7230."
update Jaeger propagator

fbogsany · 2021-03-17T19:51:28Z

I think this is ready for review. The outstanding CI failures are in the Jaeger propagator, which I (or @fhwang) still need to update.

fbogsany · 2021-03-18T02:53:39Z

I'm not sure how to square the B3 propagator requirements with the configuration options for OTEL_PROPAGATORS. The former requires the B3 propagator to

attempt to extract B3 encoded using single and multi-header formats. The single-header variant takes precedence over the multi-header version.

and

default to injecting B3 using the single-header format

while the latter only allows configuring EITHER b3 (single) OR b3multi OR both, for both injection and extraction.

I can add a B3#text_map_propagator that matches the spec requirements, but we won't have a way to set that with the environment variable. This feels like an inconsistency in the spec.

mwear · 2021-03-19T00:07:44Z

I'm not sure how to square the B3 propagator requirements with the configuration options for OTEL_PROPAGATORS.

It seems like there should be three formats for b3 in the environment variable specification. I opened open-telemetry/opentelemetry-specification#1562 to discuss options.

mwear · 2021-03-23T02:29:31Z

We discussed this at the maintainers meeting today and I opened a PR based on the discusson: open-telemetry/opentelemetry-specification#1570. tl; dr:

Option	Extract Order	Inject Format	Env var
B3 Single	Single, Multi	Single	b3
B3 Multi	Multi, Single	Multi	b3multi

fbogsany · 2021-03-23T15:35:42Z

We discussed this at the maintainers meeting today and I opened a PR based on the discusson: open-telemetry/opentelemetry-specification#1570. tl; dr:

Option Extract Order Inject Format Env var
B3 Single Single, Multi Single b3
B3 Multi Multi, Single Multi b3multi

Looks like the PR was updated to always prefer Single on extract. I think we can implement that pretty easily.

arielvalentin

Didn't have a lot to add, just some questions around symmetry and "food for thought"

arielvalentin · 2021-03-26T14:25:19Z

api/lib/opentelemetry/baggage/propagation/text_map_propagator.rb

+        # @param [optional Setter] setter If the optional setter is provided, it
+        #   will be used to write context into the carrier, otherwise the default
+        #   text map setter will be used.
+        def inject(carrier, context: Context.current, setter: Context::Propagation.text_map_setter)


How would you feel about using keyword args for all of the parameters to inject/extract?

I know for some it is idiomatic to have treated some arguments as options hash but I personally prefer keeping method arguments symmetric.

In general, we've opted to use keyword args for optional args or where a method has "many" args. In this case, the most common usage is inject(carrier). inject(carrier: carrier) doesn't increase clarity, but increases verbosity.

arielvalentin · 2021-03-26T14:25:44Z

api/lib/opentelemetry/baggage/propagation/text_map_propagator.rb

+        def inject(carrier, context: Context.current, setter: Context::Propagation.text_map_setter)
+          baggage = context[ContextKeys.baggage_key]
+
+          return if baggage.nil? || baggage.empty?


Suggested change

return if baggage.nil? || baggage.empty?

return if baggage&.empty?

The results of those expressions differ:

irb(main):001:0> b = '' => "" irb(main):002:0> b.nil? || b.empty? => true irb(main):003:0> b&.empty? => true irb(main):004:0> b = nil => nil irb(main):005:0> b.nil? || b.empty? => true irb(main):006:0> b&.empty? => nil

!!baggage&.empty? does look sad doesn't it. Oh falsey Nils!

arielvalentin · 2021-03-26T14:47:15Z

api/lib/opentelemetry/baggage/propagation/text_map_propagator.rb

+          end
+
+          context.set_value(ContextKeys.baggage_key, baggage)
+        rescue StandardError


Would we want to log errors in inject/extract to aid in debugging?

It's a good question. The concern is that it could become very noisy. We could log at a debug level. I think we're also using exceptions for control flow in some propagators (traceparent perhaps?), which is not ideal. Here we're just being defensive, so logging is probably the right thing to do.

arielvalentin · 2021-03-26T14:49:14Z

api/lib/opentelemetry/context/propagation/composite_text_map_propagator.rb

+          # @param [Array<#inject, #extract, #fields>] propagators An array of
+          #   text map propagators
+          def compose_propagators(propagators)
+            raise ArgumentError, 'propagators must be a non-nil array' unless propagators.is_a?(Array)


Should we treat Empty arrays the same as nil?

It is not technically wrong to pass an empty array here, and there are ways this could happen that result in configuration warning messages already. It might be useful instead to recognize the empty case and return a NoopTextMapPropagator.

arielvalentin · 2021-03-26T14:52:33Z

api/lib/opentelemetry/context/propagation/composite_text_map_propagator.rb

+          injectors = @injectors || @propagators
+          injectors.each do |injector|
+            injector.inject(carrier, context: context, setter: setter)
+          rescue => e # rubocop:disable Style/RescueStandardError


May I ask that we qualify the exception type (here and in other places)?

Suggested change

rescue => e # rubocop:disable Style/RescueStandardError

rescue StandardError => e

I'll do some digging to see why we do this. I feel like we had a good reason, but I don't recall what it was.

There was no good reason that I can see - fixing.

I'm only fixing the propagator-related ones in this PR. The others can be addressed in a separate PR.

arielvalentin · 2021-03-26T14:55:19Z

api/lib/opentelemetry/context/propagation/composite_text_map_propagator.rb

+          injectors.each do |injector|
+            injector.inject(carrier, context: context, setter: setter)
+          rescue => e # rubocop:disable Style/RescueStandardError
+            OpenTelemetry.logger.warn "Error in CompositePropagator#inject #{e.message}"


I believe that Logger#warn will return true. This should return nil correct?

irb(main):003:0> l = Logger.new("/dev/null") => #<Logger:0x00007fb55c02f738 @level=0, @progname=nil, @default_formatter=#<Logger::Formatter:0x00007fb55c02f6e8 @datetime_format=nil>, @formatter=nil, @logdev=#<Logger::LogDevice:0x00007fb55c02f300 @shift_period_suffix="%Y%m%d", @shift_size=1048576, @shift_age=0, @filename="/dev/null", @dev=#<File:/dev/null>, @mon_mutex=#<Thread::Mutex:0x00007fb55c02f0f8>, @mon_mutex_owner_object_id=70208454752640, @mon_owner=nil, @mon_count=0>> irb(main):004:0> l.warn "afds" => true

arielvalentin · 2021-03-26T18:39:56Z

instrumentation/http/test/instrumentation/http/patches/client_test.rb

-  after { instrumentation.instance_variable_set(:@installed, false) }
+  after do
+    # Force re-install of instrumentation
+    instrumentation.instance_variable_set(:@installed, false)


I realize this is not introduced in the PR but what is force re-reinstall doing? Should there be a reset method similar to that of exporter?

reset is only defined for the InMemorySpanExporter - it isn't part of the Exporter API - arguably that exporter should be moved out of the SDK to the common package (or somewhere else).

The force-reinstall is used in instrumentation tests (only) to avoid state leakage between tests. Re-installing instrumentation is not a supported operation in general.

fbogsany · 2021-03-30T21:23:12Z

Status: I've merged the baggage changes & resolved conflicts. The outstanding piece is updating the xray propagator. I'll try to complete that tonight or tomorrow. It is isolated, though, and won't invalidate reviews of the rest of this PR.

mwear

Thanks for the huge refactor @fbogsany!

robertlaurin

🥇

fix!: refactor propagators to add #fields

d384200

fbogsany mentioned this pull request Mar 8, 2021

fix: Ensure b3 injectors always return a carrier #644

Closed

fbogsany added 5 commits March 17, 2021 09:29

Merge branch 'main' into propagator-refactor

9c38b49

fix API tests

7c12d68

fix SDK tests

e8b7669

fix B3 tests

8a67ceb

update ottrace propagator

0eb349c

fbogsany marked this pull request as ready for review March 17, 2021 19:51

fbogsany requested review from dazuma, ericmustin, mwear and robertlaurin as code owners March 17, 2021 19:51

fbogsany and others added 5 commits March 17, 2021 15:54

add fields method to OTTrace propagator

f786820

Update Jaeger propagator. (#663)

1271c86

test fixes

0b95531

fix tests & composite prop

00ae7c7

fix composite prop test

afe557c

Merge branch 'main' into propagator-refactor

f0b6d4a

arielvalentin reviewed Mar 26, 2021

View reviewed changes

fbogsany added 4 commits March 30, 2021 10:38

fix: b3 extract priority

23332c6

fix: feedback

3b8ea38

Merge remote-tracking branch 'origin/main' into propagator-refactor

13c5787

fix: merge conflicts

2dde087

mwear approved these changes Mar 31, 2021

View reviewed changes

robertlaurin approved these changes Mar 31, 2021

View reviewed changes

fix: update XRay propagator

d417fa4

fbogsany mentioned this pull request Mar 31, 2021

Same trace_id generated for many requests #670

Closed

fbogsany merged commit 6bca875 into main Mar 31, 2021

fbogsany deleted the propagator-refactor branch March 31, 2021 15:46

robbkidd mentioned this pull request Apr 22, 2021

release: Release 35 gems #720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: refactor propagators to add #fields #638

fix!: refactor propagators to add #fields #638

fbogsany commented Mar 5, 2021 •

edited

Loading

fbogsany commented Mar 17, 2021

fbogsany commented Mar 18, 2021

mwear commented Mar 19, 2021

mwear commented Mar 23, 2021

fbogsany commented Mar 23, 2021

arielvalentin left a comment

arielvalentin Mar 26, 2021

fbogsany Mar 29, 2021

arielvalentin Mar 26, 2021

fbogsany Mar 29, 2021

arielvalentin Mar 29, 2021

arielvalentin Mar 26, 2021

fbogsany Mar 29, 2021

arielvalentin Mar 29, 2021

arielvalentin Mar 26, 2021

fbogsany Mar 29, 2021

arielvalentin Mar 26, 2021

fbogsany Mar 29, 2021

fbogsany Mar 30, 2021

fbogsany Mar 30, 2021

arielvalentin Mar 26, 2021

arielvalentin Mar 26, 2021

fbogsany Mar 29, 2021

arielvalentin Mar 29, 2021

fbogsany commented Mar 30, 2021

mwear left a comment

robertlaurin left a comment

	return if baggage.nil? \|\| baggage.empty?
	return if baggage&.empty?

	rescue => e # rubocop:disable Style/RescueStandardError
	rescue StandardError => e

fix!: refactor propagators to add #fields #638

fix!: refactor propagators to add #fields #638

Conversation

fbogsany commented Mar 5, 2021 • edited Loading

TODO

fbogsany commented Mar 17, 2021

fbogsany commented Mar 18, 2021

mwear commented Mar 19, 2021

mwear commented Mar 23, 2021

fbogsany commented Mar 23, 2021

arielvalentin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbogsany commented Mar 30, 2021

mwear left a comment

Choose a reason for hiding this comment

robertlaurin left a comment

Choose a reason for hiding this comment

fbogsany commented Mar 5, 2021 •

edited

Loading