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

[OpenTracing] Tracer start_span and start_active_span implementation #490

Merged
merged 3 commits into from Jul 17, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Jul 16, 2018

This pull request uses the other components from Datadog::OpenTrace to implement the start_span and start_active_span functions.

It does not implement the inject or extract functions, which will come in a separate pull request.

@delner delner added core Involves Datadog core libraries feature Involves a product feature labels Jul 16, 2018
@delner delner self-assigned this Jul 16, 2018
@delner delner added this to Review in OpenTracing support Jul 16, 2018
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

looks good

ignore_active_scope: ignore_active_scope
)

scope_manager.activate(span, finish_on_close: finish_on_close).tap do |scope|
Copy link
Member

Choose a reason for hiding this comment

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

👍

# References#CHILD_OF reference to the ScopeManager#active.
# @return [Span] the newly-started Span instance, which has not been
# automatically registered via the ScopeManager
def start_span(operation_name,
Copy link
Member

Choose a reason for hiding this comment

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

👍 👍

Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

Implementation and tests both look good to me 👍

@delner
Copy link
Contributor Author

delner commented Jul 17, 2018

Thanks for the reviews guys. Think I'm going to add a few more tests to verify basic features (like tags, etc.)

@delner delner force-pushed the feature/opentracing_tracer branch from b1028be to ca4a379 Compare July 17, 2018 20:47
@delner
Copy link
Contributor Author

delner commented Jul 17, 2018

Found a small bug where the ThreadLocalScopeManager wasn't passing finish_on_close to its ThreadLocalScope on #activate.

@delner delner merged commit e550983 into feature/opentracing Jul 17, 2018
delner added a commit that referenced this pull request Jul 19, 2018
…490)

 * Fixed: ThreadLocalScopeManager not passing finish_on_close to scope.

 * Added: Implementation for OpenTracer::Tracer

 * Added: OpenTracing::Tracer integration specs.
delner added a commit that referenced this pull request Aug 9, 2018
…490)

 * Fixed: ThreadLocalScopeManager not passing finish_on_close to scope.

 * Added: Implementation for OpenTracer::Tracer

 * Added: OpenTracing::Tracer integration specs.
@delner delner moved this from Review to Done in OpenTracing support Aug 9, 2018
@delner delner deleted the feature/opentracing_tracer branch August 14, 2018 17:42
delner added a commit that referenced this pull request Aug 17, 2018
…490)

 * Fixed: ThreadLocalScopeManager not passing finish_on_close to scope.

 * Added: Implementation for OpenTracer::Tracer

 * Added: OpenTracing::Tracer integration specs.
@delner delner added this to the 0.16.0 milestone Aug 23, 2018
delner added a commit that referenced this pull request Sep 13, 2018
…490)

 * Fixed: ThreadLocalScopeManager not passing finish_on_close to scope.

 * Added: Implementation for OpenTracer::Tracer

 * Added: OpenTracing::Tracer integration specs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries feature Involves a product feature
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants