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

Context prop combined #147

Merged
merged 79 commits into from
Feb 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
79 commits
Select commit Hold shift + click to select a range
864564a
Introduce basic Context object
mwear Nov 5, 2019
49cff24
Implement Context#attach and Context#detach
mwear Nov 5, 2019
7c0949e
Rename set->update; freeze entries
mwear Nov 5, 2019
ddc8cc5
Handle detach when parent is nil
mwear Nov 5, 2019
d6e107b
Add Context.with_current to handle context nesting
mwear Nov 4, 2019
4a03744
Update active span tracking to use Context
mwear Nov 5, 2019
ceca1d9
Rename Context#update -> Context.set
mwear Nov 5, 2019
b37ac9d
Add Context#clear method
mwear Nov 5, 2019
6802ee1
Introduce baggage context
mwear Nov 5, 2019
837010d
Context propagation prototype
fbogsany Nov 7, 2019
539c428
Comment out failing tests after rebase
mwear Nov 7, 2019
814cf80
Appease the cop with todos for missing docs
mwear Nov 7, 2019
e69ae76
Move baggage implementation to SDK
mwear Nov 7, 2019
0f53c8d
Use method names from API keep implicit (block) form, for now
mwear Nov 7, 2019
bc7dae9
Update baggage to use explicit context
mwear Nov 8, 2019
d72f14e
Use context keys for baggage and current span
mwear Nov 8, 2019
dc95d11
Align Context methods more closely with Java prototype
mwear Nov 8, 2019
081d6d5
Add baggage_manager accessor to OpenTelemetry Module
mwear Nov 8, 2019
44e10a3
Convert tests for distributed_context_manager to correlation_context_…
mwear Nov 8, 2019
29913b2
Add minimal CorrelationContextManager implementation
mwear Nov 9, 2019
bb699db
Add constructor and test for Label
mwear Nov 12, 2019
9d3f047
Cleanup context in an after block rather than before
mwear Nov 12, 2019
51f247f
Initial implementation of SDK correlation context
mwear Nov 13, 2019
8e8bbb2
Simplify label
mwear Nov 21, 2019
20002d2
Initial implementation of SDK::CorrelationContextManager
mwear Nov 21, 2019
282caa9
Add initial Propagation API
mwear Dec 3, 2019
7bc6872
Move SpanContext related propagation to Trace namespace
mwear Dec 3, 2019
c7143fe
Add HttpTraceContextInjector/Extractor
mwear Dec 4, 2019
cb34670
Add global injectors and extractors
mwear Dec 5, 2019
d59d1c2
Make Baggage::Manager a class instead of a module
mwear Dec 5, 2019
2f99708
Add Tracer#current_span_context
mwear Dec 6, 2019
f0d5024
Add Context.with_values
mwear Dec 6, 2019
5c85983
Use Context.with_values in Tracer#with_span
mwear Dec 7, 2019
897297d
Implement HttpTraceContextExtractor / Injector directly
mwear Dec 9, 2019
e774350
Remove TextFormat and references
mwear Dec 9, 2019
0fde9f8
Remove reference to binary format in TracerFactory
mwear Dec 9, 2019
2cfc20a
Remove references to CorrelationContext (temporarily)
mwear Dec 10, 2019
1e7c764
Add tests for context on multiple threads
mwear Dec 10, 2019
169f6b9
Provide default key names for HttpTraceContext / Injector extractor
mwear Dec 11, 2019
9cd0eb8
Standardize naming for Baggage and DistributedContext injector / extr…
mwear Dec 11, 2019
ef32bc4
Add readers for injectors / extractors to TracerFactory
mwear Dec 11, 2019
6dc3ea7
Remove distributed context
mwear Dec 12, 2019
d60acc8
Move Baggage -> CorrelationContext
mwear Dec 12, 2019
e0305ff
Prefer current span over extracted span context as implicit parent
mwear Dec 12, 2019
814e325
Propagate remote span context if there is not a current span
mwear Dec 13, 2019
83176bb
Rename remote_span_context_key -> extracted_span_context_key
mwear Dec 13, 2019
09c2f31
Move propagator references from TracerFactory to global Propagation
mwear Dec 14, 2019
6f0a04a
Happy path correlation context extractor
mwear Dec 18, 2019
2a21291
Ignore correlation context properties
mwear Dec 18, 2019
6d0c005
Initial HttpCorrelationContextInjector implmentation
mwear Dec 19, 2019
6b5411b
Manage CorrelationContext values as Strings
mwear Dec 19, 2019
e3c195a
Accept explicit context object in Tracer#start_span
mwear Dec 19, 2019
eaf04b5
Use Correlation-Context as the default key
mwear Dec 19, 2019
15c75a1
Add HttpCorrelationContextExtractor/Injectors to Propagation
mwear Dec 20, 2019
26df483
Extract default getter / setters into modules
mwear Dec 20, 2019
f43da9e
Log warning for non matching Context attach / detach
mwear Dec 20, 2019
0cfc36c
Fix @todo docs
mwear Dec 20, 2019
5402193
Use alternative context implementation
mwear Dec 10, 2019
63e5688
Remove explicit block parameters in Context
mwear Dec 21, 2019
7826be7
Provide defaults for Propagation#inject and Propagation#extract
mwear Dec 24, 2019
e926e59
Add implicit context to CorrelationContext::Manager methods
mwear Dec 24, 2019
1dfb66e
Introduce Builder to facilitate multiple modifications to Correlation…
mwear Dec 25, 2019
a7fe406
Reorganize accessors for injector / extractors
mwear Jan 9, 2020
fd2c55a
Rename HttpCorrelationContextInjector/Extractor -> HttpInjector/Extra…
mwear Jan 9, 2020
d4036a4
Cleanup ContextKeys
mwear Jan 9, 2020
9f7e580
Remove Context#attach and Context#detach
mwear Jan 10, 2020
49b6356
Cleanup correlations
mwear Jan 10, 2020
0544f73
Remove with_context from Tracer#start_span, redefine with_parent_context
mwear Jan 10, 2020
5e38da0
Return original context if parsing TraceContext fails
mwear Jan 17, 2020
df8ea06
Revert "Remove Context#attach and Context#detach"
mwear Jan 25, 2020
b3775c1
Document Context#attach and Context#detach to be private
mwear Jan 25, 2020
bb26c99
Introduce Context::Key for indexing and retrieving values from a Context
mwear Jan 25, 2020
2b62da5
Return to Hash based context
mwear Jan 25, 2020
8eb736a
Don't test implementation details
mwear Jan 25, 2020
fcc337b
Update docs
mwear Jan 25, 2020
4329947
Fix unnecessary cop after rebase
mwear Jan 25, 2020
108f9d1
Return original context on failed Correlation Context extraction
mwear Jan 25, 2020
5104720
Update Faraday and Sinatra adapters to use OpenTelemetry.propagation
mwear Jan 25, 2020
805dfc7
Rename OpenTelemetry.correlation_context_manager -> OpenTelemetry.cor…
mwear Feb 1, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,8 @@ def disable_span_reporting?(_env)

attr_reader :app

# Outbound requests should only need to inject the current span.
def propagate_context(span, env)
propagator.inject(span.context, env.request_headers)
end

def propagator
OpenTelemetry.tracer_factory.http_text_format
OpenTelemetry.propagation.inject(env.request_headers)
end

def tracer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def call(env)
attr_reader :app

def parent_context(env)
OpenTelemetry.tracer_factory.http_text_format.extract(env)
OpenTelemetry.propagation.extract(env)
end

def tracer
Expand Down
22 changes: 14 additions & 8 deletions api/lib/opentelemetry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

require 'opentelemetry/error'
require 'opentelemetry/context'
require 'opentelemetry/distributed_context'
require 'opentelemetry/correlation_context'
require 'opentelemetry/internal'
require 'opentelemetry/instrumentation'
require 'opentelemetry/metrics'
Expand All @@ -23,7 +23,7 @@
module OpenTelemetry
extend self

attr_writer :tracer_factory, :meter_factory, :distributed_context_manager
attr_writer :tracer_factory, :meter_factory, :correlations

attr_accessor :logger

Expand All @@ -39,17 +39,23 @@ def meter_factory
@meter_factory ||= Metrics::MeterFactory.new
end

# @return [Object, DistributedContext::Manager] registered distributed
# context manager or a default no-op implementation of the manager
def distributed_context_manager
@distributed_context_manager ||= DistributedContext::Manager.new
end

# @return [Instrumentation::Registry] registry containing all known
# instrumentation
def instrumentation_registry
@instrumentation_registry ||= Instrumentation::Registry.new
end

# @return [Object, CorrelationContext::Manager] registered
# correlation context manager or a default no-op implementation of the
# manager.
def correlations
@correlations ||= CorrelationContext::Manager.new
end

# @return [Context::Propagation::Propagation] an instance of the propagation API
def propagation
@propagation ||= Context::Propagation::Propagation.new
end

self.logger = Logger.new(STDOUT)
end
148 changes: 133 additions & 15 deletions api/lib/opentelemetry/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,146 @@
#
# SPDX-License-Identifier: Apache-2.0

require 'opentelemetry/context/key'
require 'opentelemetry/context/propagation'

Choose a reason for hiding this comment

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

I'm still trying to wrap my head around this whole thing but I'm a little surprised to see this dep because the otep author seems to indicate

The Propagator API knows about the Context API, but the Context API does not need to know about Propagation.

https://github.com/open-telemetry/oteps/pull/66/files#r359638018

Copy link
Member Author

Choose a reason for hiding this comment

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

The Context API is actually separate. That require is for namespacing reasons, so that all propagation code nests under OpenTelemetry::Context::Propagation. Since OpenTelemetry::Context doesn't actually depend on anything in OpenTelemetry::Context::Propagation, we could consider moving propagation up to the top level, OpenTelemetry::Propagation. However, propagation does depend on context, so there is a relationship there.


module OpenTelemetry
# The Context module provides per-thread storage.
module Context
extend self
# Manages context on a per-fiber basis
class Context
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking some more about this, the Context API from the OTEP differs in an important way from this implementation: it recommends a CreateKey(name) -> key function that returns a key for a name that is subsequently used to get, set and remove values.

I think the addition of this abstraction might allow a more efficient underlying implementation.

The OTEP describes two cross-cutting concerns (Observability and Correlations), while the Context API is generic enough to support additional concerns outside of the two specified. I think there is value in leveraging knowledge of the two specified concerns to improve lookup efficiency. For example, giving Context three fields:

@observability = nil
@correlations = nil
@extensions = {}.freeze

and copying the fields in attach rather than creating a linked list.

My assumption here is that we can define a private class that contains all the relevant context for Observability and likewise for Correlations. For Correlations, that may well be just a frozen Hash.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Context object itself should be a general purpose context object that is not tied to observability or even OpenTelemetry. There has been some talk of extracting Context into a separate package at some point in the future. Whether or not that will happen remains to be seen, but we should design with that goal in mind.

i went ahead and reverted back to my original frozen hash design to alleviate concerns over the linked list approach. I also implemented Context#create_key and am using Context::Keys for indexing entries. I think this gives us what we need and keeps the context decoupled from observability.

KEY = :__opentelemetry_context__
EMPTY_ENTRIES = {}.freeze

class << self
# Returns a key used to index a value in a Context
#
# @param [String] name The key name
# @return [Context::Key]
def create_key(name)
Key.new(name)
end

# Returns current context, which is never nil
#
# @return [Context]
def current
Thread.current[KEY] ||= ROOT
end

# Sets the current context
#
# @param [Context] ctx The context to be made active
def current=(ctx)
Thread.current[KEY] = ctx
end

# Executes a block with ctx as the current context. It restores
# the previous context upon exiting.
#
# @param [Context] ctx The context to be made active
def with_current(ctx)
prev = ctx.attach
yield
ensure
ctx.detach(prev)
end

# Execute a block in a new context with key set to value. Restores the
# previous context after the block executes.

mwear marked this conversation as resolved.
Show resolved Hide resolved
# @param [String] key The lookup key
# @param [Object] value The object stored under key
# @param [Callable] Block to execute in a new context
def with_value(key, value)
ctx = current.set_value(key, value)
prev = ctx.attach
yield value
ensure
ctx.detach(prev)
end

# Execute a block in a new context where its values are merged with the
# incoming values. Restores the previous context after the block executes.

mwear marked this conversation as resolved.
Show resolved Hide resolved
# @param [String] key The lookup key
# @param [Hash] values Will be merged with values of the current context
# and returned in a new context
# @param [Callable] Block to execute in a new context
def with_values(values)
ctx = current.set_values(values)
prev = ctx.attach
yield values
ensure
ctx.detach(prev)
end

# Returns the value associated with key in the current context
#
# @param [String] key The lookup key
def value(key)
current.value(key)
end

def clear
self.current = ROOT
end

def get(key)
storage[key]
def empty
new(nil, EMPTY_ENTRIES)
end
end

def with(key, value)
store = storage
previous = store[key]
store[key] = value
yield value
ensure
store[key] = previous
def initialize(parent, entries)
@parent = parent
@entries = entries.freeze
end

private
# Returns the corresponding value (or nil) for key
#
# @param [Key] key The lookup key
# @return [Object]
def value(key)
@entries[key]
end

alias [] value

# Returns a new Context where entries contains the newly added key and value
#
# @param [Key] key The key to store this value under
# @param [Object] value Object to be stored under key
# @return [Context]
def set_value(key, value)
new_entries = @entries.dup
new_entries[key] = value
Context.new(self, new_entries)
end

# Returns a new Context with the current context's entries merged with the
# new entries
#
# @param [Hash] values The values to be merged with the current context's
# entries.
# @param [Object] value Object to be stored under key
# @return [Context]
def set_values(values) # rubocop:disable Naming/AccessorMethodName:
Context.new(self, @entries.merge(values))
end

def storage
Thread.current[:__opentelemetry__] ||= {}
# @api private
def attach
prev = self.class.current
self.class.current = self
prev
end

# @api private
def detach(ctx_to_attach = nil)
OpenTelemetry.logger.warn 'Calls to detach should match corresponding calls to attach' if self.class.current != self

ctx_to_attach ||= @parent || ROOT
ctx_to_attach.attach
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't read too far into this PR yet, so I may be missing some context (ha!) here. The design of this class makes me uncomfortable.

  • detach(prev) seems unnecessary when we can just call prev.attach
  • we have 2 forms of nesting: the implicit linked list of single-valued Contexts, and the current and previously attached Contexts on the Fiber's stack, and the 2 forms are not necessarily related
  • single-valued Contexts can result in havoc when set_values(hash) is combined with detach(nil), where detach(nil) will attach the Context without the last value in hash rather than the receiver of set_values(hash) (with_values helps with this, but exposing both methods makes it easier for people to shoot themselves in the foot)
  • the linked list of single-valued Contexts makes lookup proportional to the number of values set in a codepath, which is potentially a lot more than the count of active values (a value can be set for a key many times, and each time increases the cost to access values for other keys set earlier)
  • exposing both block-structured contexts and explicit attachment is potentially very error prone, based on my experience with a similar mechanism in Shopify's tracing gem (I strongly prefer only supporting block-structured contexts).

Copy link
Member Author

Choose a reason for hiding this comment

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

These are all valid concerns and I share most of them.

I don't think that attach and detach should be exposed if we can avoid it. I'll double check and see, but we might be able to remove them. Other options would be to put an @api private comment, or if we must expose them for edge cases, we can put a nasty gram in the comment to use at your own risk. They were inspired by this prior art: https://github.com/grpc/grpc-java/blob/master/context/src/main/java/io/grpc/Context.java#L405-L449.

I borrowed the single valued contexts idea from go. Take a look at line 480 until the end of the file: https://golang.org/src/context/context.go. You can also take a look at the hash based context I had earlier in this PR: a226469. Let me know if you'd be more comfortable with one over the other.

Copy link
Member Author

@mwear mwear Jan 10, 2020

Choose a reason for hiding this comment

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

I went ahead and removed Context#attach and Context#detach. They aren't necessary for the block form, and if we find they become necessary, we can bring them back later with caveats and warnings.


ROOT = empty.freeze
end
end
29 changes: 29 additions & 0 deletions api/lib/opentelemetry/context/key.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# frozen_string_literal: true

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

module OpenTelemetry
class Context
# The Key class provides mechanisms to index and access values from a
# Context
class Key
attr_reader :name

# @api private
# Use Context.create_key to obtain a Key instance.
def initialize(name)
@name = name
end

# Returns the value indexed by this Key in the specified context
#
# @param [optional Context] context The Context to lookup the key from.
# Defaults to +Context.current+.
def get(context = Context.current)
context[self]
end
end
end
end
18 changes: 18 additions & 0 deletions api/lib/opentelemetry/context/propagation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

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

require 'opentelemetry/context/propagation/default_getter'
require 'opentelemetry/context/propagation/default_setter'
require 'opentelemetry/context/propagation/propagation'
mwear marked this conversation as resolved.
Show resolved Hide resolved

module OpenTelemetry
class Context
# The propagation module contains APIs and utilities to interact with context
# and propagate across process boundaries.
module Propagation
end
end
end
26 changes: 26 additions & 0 deletions api/lib/opentelemetry/context/propagation/default_getter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# frozen_string_literal: true

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

module OpenTelemetry
class Context
module Propagation
# The default getter module provides a common method for reading
# a key from a carrier that implements +[]+
module DefaultGetter
DEFAULT_GETTER = ->(carrier, key) { carrier[key] }
private_constant :DEFAULT_GETTER

# Returns a callable that can read a key from a carrier that implements
# +[]+. Useful for extract operations.
#
# @return [Callable]
def default_getter
DEFAULT_GETTER
end
end
end
end
end
26 changes: 26 additions & 0 deletions api/lib/opentelemetry/context/propagation/default_setter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# frozen_string_literal: true

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

module OpenTelemetry
class Context
module Propagation
# The default setter module provides a common method for writing
# a key into a carrier that implements +[]=+
module DefaultSetter
DEFAULT_SETTER = ->(carrier, key, value) { carrier[key] = value }
private_constant :DEFAULT_SETTER

# Returns a callable that can write a key into a carrier that implements
# +[]=+. Useful for inject operations.
#
# @return [Callable]
def default_setter
DEFAULT_SETTER
end
end
end
end
end
Loading