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.

Sign up for GitHub

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

Jump to bottom

[concurrent_ruby] Propagate active context into span Future whose execution is being deferred. #496

Merged
merged 11 commits into from
Sep 11, 2018
Merged

[concurrent_ruby] Propagate active context into span Future whose execution is being deferred. #496

merged 11 commits into from
Sep 11, 2018

Conversation

pawelchcki
Copy link
Contributor

@pawelchcki pawelchcki commented Jul 20, 2018 •
edited

Implementation of Context passing to Future execution

Concurrent Ruby

The Concurrent Ruby integration adds support for context propagation when using ::Concurrent::Future.
Making sure that code traced within the Future#execute will have correct parent set.

To activate your integration, use the Datadog.configure method:

# Inside Rails initializer or equivalent
Datadog.configure do |c|
  c.use :concurrent_ruby # patches ::Concurrent::Future to use ExecutorService that propagates context
end

# Pass context into code executed within Concurrent::Future
Datadog.tracer.trace('outer') do
  Concurrent::Future.execute { Datadog.tracer.trace('inner') { } }.wait
end

The use :concurrent_ruby method accepts the following parameters:

Key Description Default
tracer A Datadog::Tracer instance used to instrument the application. Usually you don't need to set that. Datadog.tracer

@pawelchcki pawelchcki self-assigned this Jul 20, 2018
@pawelchcki pawelchcki changed the title Feature/futures future Propagate active context into span Future whose execution is being deferred. Jul 20, 2018
@pawelchcki pawelchcki requested a review from delner July 20, 2018 18:35
@pawelchcki pawelchcki added this to the 0.14.0 milestone Jul 20, 2018
@pawelchcki pawelchcki added integrations Involves tracing integrations feature Involves a product feature labels Jul 20, 2018
@pawelchcki pawelchcki changed the title Propagate active context into span Future whose execution is being deferred. [concurrent_ruby] Propagate active context into span Future whose execution is being deferred. Jul 20, 2018
@pawelchcki
Copy link
Contributor Author

@delner can you review this PR ?

@delner delner modified the milestones: 0.14.0, 0.15.0 Aug 7, 2018
@delner
Copy link
Contributor

delner commented Aug 7, 2018

Bumping this to 0.15.0.

@delner
Copy link
Contributor

delner commented Aug 9, 2018

@pawelchcki Can you resolve the merge conflict with the GettingStarted.md?

delner
delner reviewed Aug 9, 2018
View reviewed changes
Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

Overall so far, looking good. Just some general questions so I can understand it a bit better.

lib/ddtrace/contrib/concurrent_ruby/integration.rb
register_as :concurrent_ruby

def self.compatible?
defined?(::Concurrent::Future)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it compatible with Ruby 1.9-2.5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

lib/ddtrace/contrib/concurrent_ruby/context_composite_executor_service.rb Outdated
@composited_executor = composited_executor
end

def post(*args, &task)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain a little about what this function does? Its purpose? Presumably in the context of Concurrent Ruby?

spec/ddtrace/contrib/concurrent_ruby/integration_spec.rb Outdated

subject(:deferred_execution) do
outer_span = tracer.trace('outer_span')
inner_span = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious; why does this need to be set to nil here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be able to set the variable in the Closure below and make it accessible outside

spec/ddtrace/contrib/concurrent_ruby/integration_spec.rb Outdated

around do |example|
unmodified = ::Concurrent::Future.dup
Datadog.registry[:concurrent_ruby].patcher.instance_variable_set(:@done_once, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see how you might want to redo patching when testing patching. However, does this need to be at the outermost level? Can this unpatching be done only in the patching context? Generally speaking we want to avoid hacking our patching like this, so limiting its footprint to only where its absolutely necessary would be best.

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

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 had to revert this change as all tests required the Future to be in known state as otherwise it would fail based on the order on execution

@pawelchcki pawelchcki changed the base branch from 0.14-dev to 0.15-dev August 23, 2018 19:14
@pawelchcki
Copy link
Contributor Author

Resolved the conflicts and address the code review suggestions
@delner this PR is now ready for another round (once tests will pass)

delner
delner approved these changes Sep 10, 2018
View reviewed changes
Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

👍

@pawelchcki pawelchcki merged commit f2a5d69 into 0.15-dev Sep 11, 2018
@pawelchcki pawelchcki deleted the feature/futures_support branch September 11, 2018 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delner delner delner approved these changes

Assignees

@pawelchcki pawelchcki

Labels
feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pawelchcki @delner