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 support for ActionController::Live #772

Closed
Closed
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ def gem_version

def patch
Handlers.subscribe
ActionController::Live.include(Patches::ActionController::Live)
end

def require_dependencies
require_relative 'handlers'
require_relative 'patches/action_controller/live'
end

def require_railtie
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# frozen_string_literal: true

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

module OpenTelemetry
module Instrumentation
module ActionPack
module Patches
module ActionController
# Module to append to ActionController::Live for instrumentation
module Live
def process_action(*)
current_context = OpenTelemetry::Context.current

# Unset thread local to avoid modifying stack array shared with parent thread
Thread.current[:__opentelemetry_context_storage__] = nil
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

This is bad. We shouldn't have to reach into the API internals in this way. This happens because OpenTelemetry::Context.clear is implemented as stack.clear, which empties the backing array instead of replacing it with a new empty instance.

I think it'd be worthwhile to change the implementation of OpenTelemetry::Context.clear to be Thread.current[STACK_KEY] = [].

On the other hand, I think there is a race condition when fetching the current context. Specifically, since the thread-locals are copied, the backing array for the context stack is shared with the parent thread, and if that parent thread modifies its context before this thread retrieves it, the context used in this thread will be incorrect.

Side note: the tracing library we built at Shopify before OpenTelemetry Ruby patched ActionController::Live in a very similar way, other than unsetting the thread local. It worked there because the thread-local was a Span instead of an array, and we managed the "stack" as a linked list of spans.

I think it might be preferable to explicitly propagate context in some way, but I'm not familiar enough with ActionController::Live to know how to accomplish that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could leverage/patch ActiveSupport::IsolatedExecutionState.share_with in some way? 🤔

That would be a good place to "propagate" the context, probably (if we fix Context.clear) by:

current_context = OpenTelemetry::Context.current
OpenTelemetry::Context.clear
OpenTelemetry::Context.attach(current_context)

It's a bit of a hack, in that we're not going to Context.detach. The patch for Live could call OpenTelemetry::Context.clear after the in_span call.

I.e. our patch for ActiveSupport::IsolatedExecutionState.share_with would be:

def share_with(other)
  super
  current_context = OpenTelemetry::Context.current
  OpenTelemetry::Context.clear
  OpenTelemetry::Context.attach(current_context)
end

And our patch for Live would be simply:

attributes = {
  OpenTelemetry::SemanticConventions::Trace::CODE_NAMESPACE => self.class.name,
  OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => action_name
}
Instrumentation.instance.tracer.in_span("#{self.class.name}##{action_name} stream", attributes: attributes) do
  super
end
OpenTelemetry::Context.clear

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @fbogsany. I will open a PR in opentelemetry-ruby to propose your suggested change to Context.clear. I'll also probably close this PR and reopen a new one with a different approach, using explicit propagation like you suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried a linked list implementation of the context stack in open-telemetry/opentelemetry-ruby#1597 but the performance impact was significant (~9% with the interpreter, as much as 24% with yjit).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

open-telemetry/opentelemetry-ruby#1598 is merged. Once we cut a new release of the API, we can use that as a minimum version here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thomasmarshall - A new version of the API has been released! Could you incorporate it into this PR?


attributes = {
OpenTelemetry::SemanticConventions::Trace::CODE_NAMESPACE => self.class.name,
OpenTelemetry::SemanticConventions::Trace::CODE_FUNCTION => action_name
}

OpenTelemetry::Context.with_current(current_context) do
Instrumentation.instance.tracer.in_span("#{self.class.name}##{action_name} stream", attributes: attributes) do
super
end
end
end
end
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# frozen_string_literal: true

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

require 'test_helper'

describe OpenTelemetry::Instrumentation::ActionPack::Patches::ActionController::Live do
include Rack::Test::Methods

let(:instrumentation) { OpenTelemetry::Instrumentation::ActionPack::Instrumentation.instance }
let(:exporter) { EXPORTER }
let(:spans) { exporter.finished_spans }
let(:span) { exporter.finished_spans.last }
let(:rails_app) { DEFAULT_RAILS_APP }
let(:config) { {} }

# Clear captured spans
before do
OpenTelemetry::Instrumentation::ActionPack::Handlers.unsubscribe

instrumentation.instance_variable_set(:@config, config)
instrumentation.instance_variable_set(:@installed, false)

instrumentation.install(config)

exporter.reset
end

it 'creates a child span for the new thread' do
get '/stream'

parent_span = spans[-2]

_(last_response.ok?).must_equal true
_(span.name).must_equal 'ExampleLiveController#stream stream'
_(span.kind).must_equal :internal
_(span.status.ok?).must_equal true

_(span.instrumentation_library.name).must_equal 'OpenTelemetry::Instrumentation::ActionPack'
_(span.instrumentation_library.version).must_equal OpenTelemetry::Instrumentation::ActionPack::VERSION

_(span.attributes['code.namespace']).must_equal 'ExampleLiveController'
_(span.attributes['code.function']).must_equal 'stream'

_(span.parent_span_id).must_equal parent_span.span_id
end

def app
rails_app
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@
# SPDX-License-Identifier: Apache-2.0

require_relative 'controllers/example_controller'
require_relative 'controllers/example_live_controller'
require_relative 'controllers/exceptions_controller'
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

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

class ExampleLiveController < ActionController::Base
include ActionController::Live

def stream
response.headers['Content-Type'] = 'text/event-stream'
10.times do
response.stream.write "hello world\n"
sleep 0.1
end
ensure
response.stream.close
end
end
1 change: 1 addition & 0 deletions instrumentation/action_pack/test/test_helpers/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ def draw_routes(rails_app)
get '/items/new', to: 'example#new_item'
get '/items/:id', to: 'example#item'
get '/internal_server_error', to: 'example#internal_server_error'
get '/stream', to: 'example_live#stream'
end
end
Loading