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

fix!: refactor propagators to add #fields #638

Merged
merged 17 commits into from
Mar 31, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions api/lib/opentelemetry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ def baggage

# @return [Context::Propagation::Propagator] a propagator instance
def propagation
@propagation ||= Context::Propagation::Propagator.new(
Context::Propagation::NoopInjector.new,
Context::Propagation::NoopExtractor.new
)
@propagation ||= Context::Propagation::NoopTextMapPropagator.new
end
end
27 changes: 9 additions & 18 deletions api/lib/opentelemetry/baggage/propagation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,23 @@
# SPDX-License-Identifier: Apache-2.0

require 'opentelemetry/baggage/propagation/context_keys'
require 'opentelemetry/baggage/propagation/text_map_injector'
require 'opentelemetry/baggage/propagation/text_map_extractor'
require 'opentelemetry/baggage/propagation/text_map_propagator'

module OpenTelemetry
module Baggage
# The Baggage::Propagation module contains injectors and
# extractors for sending and receiving baggage over the wire
# The Baggage::Propagation module contains a text map propagator for
# sending and receiving baggage over the wire.
module Propagation
extend self

BAGGAGE_KEY = 'baggage'
TEXT_MAP_EXTRACTOR = TextMapExtractor.new
TEXT_MAP_INJECTOR = TextMapInjector.new
TEXT_MAP_PROPAGATOR = TextMapPropagator.new

private_constant :BAGGAGE_KEY, :TEXT_MAP_INJECTOR, :TEXT_MAP_EXTRACTOR
private_constant :TEXT_MAP_PROPAGATOR

# Returns an extractor that extracts context using the W3C Baggage
# format
def text_map_injector
TEXT_MAP_INJECTOR
end

# Returns an injector that injects context using the W3C Baggage
# format
def text_map_extractor
TEXT_MAP_EXTRACTOR
# Returns a text map propagator that propagates context using the
# W3C Baggage format.
def text_map_propagator
TEXT_MAP_PROPAGATOR
end
end
end
Expand Down
58 changes: 0 additions & 58 deletions api/lib/opentelemetry/baggage/propagation/text_map_extractor.rb

This file was deleted.

76 changes: 0 additions & 76 deletions api/lib/opentelemetry/baggage/propagation/text_map_injector.rb

This file was deleted.

109 changes: 109 additions & 0 deletions api/lib/opentelemetry/baggage/propagation/text_map_propagator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
# frozen_string_literal: true

# Copyright The OpenTelemetry Authors
#
# SPDX-License-Identifier: Apache-2.0

require 'cgi'

module OpenTelemetry
module Baggage
module Propagation
# Propagates baggage using the W3C Baggage format
class TextMapPropagator
# Maximums according to W3C Baggage spec
MAX_ENTRIES = 180
MAX_ENTRY_LENGTH = 4096
MAX_TOTAL_LENGTH = 8192

BAGGAGE_KEY = 'baggage'
FIELDS = [BAGGAGE_KEY].freeze

private_constant :BAGGAGE_KEY, :FIELDS, :MAX_ENTRIES, :MAX_ENTRY_LENGTH, :MAX_TOTAL_LENGTH

# Inject in-process baggage into the supplied carrier.
#
# @param [Carrier] carrier The mutable carrier to inject baggage into
# @param [Context] context The context to read baggage from
# @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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

baggage = OpenTelemetry.baggage.raw_entries(context: context)

return if baggage.nil? || baggage.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return if baggage.nil? || baggage.empty?
return if baggage&.empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


encoded_baggage = encode(baggage)
setter.set(carrier, BAGGAGE_KEY, encoded_baggage) unless encoded_baggage&.empty?
nil
end

# Extract remote baggage from the supplied carrier.
# If extraction fails, the original context will be returned
#
# @param [Carrier] carrier The carrier to get the header from
# @param [optional Context] context Context to be updated with the baggage
# extracted from the carrier. Defaults to +Context.current+.
# @param [optional Getter] getter If the optional getter is provided, it
# will be used to read the header from the carrier, otherwise the default
# text map getter will be used.
#
# @return [Context] context updated with extracted baggage, or the original context
# if extraction fails
def extract(carrier, context: Context.current, getter: Context::Propagation.text_map_getter)
header = getter.get(carrier, BAGGAGE_KEY)

entries = header.gsub(/\s/, '').split(',')

OpenTelemetry.baggage.build(context: context) do |builder|
entries.each do |entry|
# Note metadata is currently unused in OpenTelemetry, but is part
# the W3C spec where it's referred to as properties. We preserve
# the properties (as-is) so that they can be propagated elsewhere.
kv, meta = entry.split(';', 2)
k, v = kv.split('=').map!(&CGI.method(:unescape))
builder.set_value(k, v, metadata: meta)
end
end
rescue StandardError => e
OpenTelemetry.logger.debug "Error extracting W3C baggage: #{e.message}"
context
end

# Returns the predefined propagation fields. If your carrier is reused, you
# should delete the fields returned by this method before calling +inject+.
#
# @return [Array<String>] a list of fields that will be used by this propagator.
def fields
FIELDS
end

private

def encode(baggage)
result = +''
encoded_count = 0
baggage.each_pair do |key, entry|
break unless encoded_count < MAX_ENTRIES

encoded_entry = encode_value(key, entry)
next unless encoded_entry.size <= MAX_ENTRY_LENGTH &&
encoded_entry.size + result.size <= MAX_TOTAL_LENGTH

result << encoded_entry << ','
encoded_count += 1
end
result.chop!
end

def encode_value(key, entry)
result = +"#{CGI.escape(key.to_s)}=#{CGI.escape(entry.value.to_s)}"
# We preserve metadata recieved on extract and assume it's already formatted
# for transport. It's sent as-is without further processing.
result << ";#{entry.metadata}" if entry.metadata
result
end
end
end
end
end
40 changes: 35 additions & 5 deletions api/lib/opentelemetry/context/propagation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,48 @@
#
# SPDX-License-Identifier: Apache-2.0

require 'opentelemetry/context/propagation/composite_propagator'
require 'opentelemetry/context/propagation/noop_extractor'
require 'opentelemetry/context/propagation/noop_injector'
require 'opentelemetry/context/propagation/propagator'
require 'opentelemetry/context/propagation/composite_text_map_propagator'
require 'opentelemetry/context/propagation/noop_text_map_propagator'
require 'opentelemetry/context/propagation/rack_env_getter'
require 'opentelemetry/context/propagation/text_map_getter'
require 'opentelemetry/context/propagation/text_map_propagator'
require 'opentelemetry/context/propagation/text_map_setter'
require 'opentelemetry/context/propagation/rack_env_getter'

module OpenTelemetry
class Context
# The propagation module contains APIs and utilities to interact with context
# and propagate across process boundaries.
#
# The API implicitly defines 3 interfaces: TextMapPropagator, TextMapInjector
# and TextMapExtractor. Concrete implementations of TextMapPropagator are
# provided. Custom text map propagators can leverage these implementations
# or simply implement the expected interface. The interfaces are described
# below.
#
# The TextMapPropagator interface:
#
# inject(carrier, context:, setter:)
# extract(carrier, context:, getter:) -> Context
# fields -> Array<String>
#
# The TextMapInjector interface:
#
# inject(carrier, context:, setter:)
# fields -> Array<String>
#
# The TextMapExtractor interface:
#
# extract(carrier, context:, getter:) -> Context
#
# The API provides 3 TextMapPropagator implementations:
# - A default NoopTextMapPropagator that implements +inject+ and +extract+
# methods as no-ops. Its +fields+ method returns an empty list.
# - A TextMapPropagator that composes an Injector and an Extractor. Its
# +fields+ method delegates to the provided Injector.
# - A CompositeTextMapPropagator that wraps either a list of text map
# propagators or a list of Injectors and a list of Extractors. Its
# +fields+ method returns the union of fields returned by the Injectors
# it wraps.
module Propagation
extend self

Expand Down
Loading