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

Allow DDTracerResolver to be disabled. #415

Merged
merged 2 commits into from
Jul 31, 2018
Merged

Allow DDTracerResolver to be disabled. #415

merged 2 commits into from
Jul 31, 2018

Conversation

tylerbenson
Copy link
Contributor

Fixes #408

@tylerbenson tylerbenson added this to the 0.12.0 milestone Jul 30, 2018
@tylerbenson tylerbenson requested a review from realark July 30, 2018 04:56
@tylerbenson tylerbenson mentioned this pull request Jul 30, 2018
Closed
@jeacott1
Copy link

thanks. this looks like it will still auto create a NoopTracer though in the case that
dd.trace.resolver.enabled=false.

perhaps it would be better to not generate or register anything at all here?

mar-kolya
mar-kolya reviewed Jul 30, 2018
View reviewed changes
dd-trace-ot/src/main/java/datadog/trace/common/util/Config.java
}
return Collections.unmodifiableMap(map);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have a dedicated test coverage for this class, especially considering that it is now public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the tests in DDTraceConfigTest still apply. I'm going to leave it as is for now to avoid further conflicts with your PR.

@mar-kolya mar-kolya mentioned this pull request Jul 30, 2018
Merged
@tylerbenson
Copy link
Contributor Author

@jeacott1 the registerTracer method that returns a NoopTracer is one we've added. The method called by TraceResolver will just return null when disabled, which I believe is the behavior you're looking for.

realark
realark approved these changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@realark realark left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeacott1
Copy link

ah, ok. I thought something else was calling registerTracer so one would be created regardless.
looks ok.

jeacott1
jeacott1 approved these changes Jul 31, 2018
View reviewed changes
Copy link

@jeacott1 jeacott1 left a comment

Choose a reason for hiding this comment

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

looks to do the job :)

@tylerbenson tylerbenson merged commit 8dd87a8 into master Jul 31, 2018
@tylerbenson tylerbenson deleted the tyler/resolver-config branch July 31, 2018 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mar-kolya mar-kolya mar-kolya left review comments

@jeacott1 jeacott1 jeacott1 approved these changes

@realark realark realark approved these changes

Assignees
No one assigned
Labels
type: enhancement
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tylerbenson @jeacott1 @mar-kolya @realark