search

opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
508 stars 56 forks source link

Adds $closeSpanOnFinish as second parameter for activate #60

Closed jcchavezs closed 6 years ago

jcchavezs commented 6 years ago

Background

Tracer::startActiveSpan starts and span that is captured in a scope and one of the SpanOptions allows you to determine whether the Scope will be closed when finishing the span by passing ['close_span_on_finish' => true].

Problem

For usability matters, $closeSpanOnFinish should allow you to be passed when doing ScopeManager::activate. Those cases occur when starting an Span not as an active one and in such a case it is impossible to pass a value for close_span_on_finish.

Proposal

Changes ScopeManager::activate(OpenTracingSpan $span) into ScopeManager::activate(OpenTracingSpan $span, $closeSpanOnFinish) so you can also decide to pass the $closeSpanOnFinish when activating a span in a Scope.

Java already does this: https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/ScopeManager.java#L36

Ping @yurishkuro @tedsuo @beberlei @ellisv

jcchavezs commented 6 years ago

Ping @eligijusvitkauskas-home24 @avsej @felixfbecker

ellisv commented 6 years ago

Because of https://github.com/opentracing/opentracing-php/commit/3010347c7329578653546c0f4319ef34ed16aa84 commit we have inverted so that Span finish is able to close Scope (thought I am personally not a fan of as we leak Scope concept to Spans). But current option name is misleading. Spans we finish, Scopes we close. So currently an option should be named something like close_scope_on_finish.

Now that a Span has to be aware of a Scope in order to close it this might be problematic as inside of ScopeManager::activate() we accept Span interface which does not have any methods so that we can pass the scope to it.

Ideally for me I would rather go back to concept of startActiveSpan() returning a Scope to not leak Scopes to Spans as it is done in other OT libs.

But in order to keep everything as it is now you would also need to introduce a method in Span interface named something like setScopeToClose()

yurishkuro commented 6 years ago

If span is aware of the scope, then I think there was a mistake in the design. Java has finishOnClose ie "finish underlying span on scope close". I think this API needs that as well.

jcchavezs commented 6 years ago

@ellisv @yurishkuro I opened a PR to address this issue: https://github.com/opentracing/opentracing-php/pull/61/files

jcchavezs commented 6 years ago

Closed by https://github.com/opentracing/opentracing-php/pull/61