Skip to content

Introduce a ScopeManager property to DDTracer#223

Merged
tylerbenson merged 1 commit into
DataDog:configurable-scope-managerfrom
jonmort:issue-222-configurable-scope-manager
Feb 15, 2018
Merged

Introduce a ScopeManager property to DDTracer#223
tylerbenson merged 1 commit into
DataDog:configurable-scope-managerfrom
jonmort:issue-222-configurable-scope-manager

Conversation

@jonmort
Copy link
Copy Markdown
Contributor

@jonmort jonmort commented Feb 8, 2018

Note that this is a binary incompatible change. I could not work out how to make this change without breaking binary compatibility.

@jonmort jonmort changed the title Closes #222: Introduce a ScopeManager property Introduce a ScopeManager property Feb 8, 2018
@jonmort jonmort changed the title Introduce a ScopeManager property Introduce a ScopeManager property to DDTracer Feb 8, 2018
@tylerbenson
Copy link
Copy Markdown
Contributor

@jonmort Thanks for your contribution! Let me talk this over with @realark. Can you share a bit more about your use case? What async framework are you trying to instrument?

@jonmort
Copy link
Copy Markdown
Contributor Author

jonmort commented Feb 9, 2018

I'm trying to instrument Ratpack. There is already an implementation for zipkin (https://github.com/hyleung/ratpack-zipkin) but I would like an opentracing implementation, integrating Datadog APM, because we already use metrics and logs.

In Ratpack scopes need to be associated with Executions rather than Threads as an Execution may run on many different threads. Most Ratpack applications will have a small number of compute threads that do CPU bound tasks and a larger pool of blocking threads for any blocking tasks. Most IO is done using async IO and the APIs are promise based.

I tried getting the current code to work, but found I had to copy DDTracer to get anywhere.

Once I have the Ratpack support complete I'll open source it along with instructions on how to integrate with Datadog.

@tylerbenson
Copy link
Copy Markdown
Contributor

@jonmort awesome! Any chance I could talk you into submitting your ratpack instrumentation as a PR as well? I'm actually a big ratpack fan. Some of our tests in this repo use ratpack as an easy embedded http server.

As for this PR, the main thing we're debating on is if we want to expose this, or just make the default scope manager know how to handle async and do reference counting. Would you mind providing a gist of the scope manager you would be using with the tracer instead?

The main concern with allowing alternate scope managers being the more surface area we open up, the harder it is for troubleshooting and future change.

@tylerbenson tylerbenson added comp: core Tracer core tag: community Community contribution labels Feb 14, 2018
@tylerbenson tylerbenson changed the base branch from master to configurable-scope-manager February 15, 2018 05:47
@tylerbenson
Copy link
Copy Markdown
Contributor

I'm going to merge this onto a branch named configurable-scope-manager and continue working off it. Feel free to follow along.

@tylerbenson tylerbenson added this to the 0.4.0 milestone Feb 15, 2018
@tylerbenson tylerbenson merged this pull request into DataDog:configurable-scope-manager Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: core Tracer core tag: community Community contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants