-
Notifications
You must be signed in to change notification settings - Fork 56
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
In process propagation #20
Conversation
Any reason startActiveSpan can't be called startSpan, rather than deprecate the method? |
4e58204
to
a0e7d6d
Compare
Fair enough. I'd prefer we hadn't called it StartActiveSpan there either, but perhaps that ship has sailed... |
Ping @jonahgeorge |
a0e7d6d
to
2dd6fda
Compare
We have two options here:
Whatever we choose I believe we will be ready to release Ping @tPl0ch @tedsuo @felixfbecker @beberlei @yurishkuro @lvht @jonahgeorge @reimertz |
1️⃣ |
@jcchavezs In my opinion, only |
I think we should only offer the most essential API. Less is more, and less is better. The opentracing specific even does not define a trace-id!!! It only defined span, tag, context, and reference. It's enough. How to trace all spans with different references is all about implementation. So should our php interface. |
@jcchavezs Any thing not specified in the OT spec should never be part of our opentracing-php API. Thank you. |
I would also prefer renaming |
I would like to keep a method for manual span creation around as a fallback to use in an environment that needs to rely on manual span propagation. For example, I currently use https://github.com/sabre-io/event as an event loop, which doesn't have hooks to make sure the active span is correctly set for each event loop tick / context. So I have to resort back to passing around the Span objects as parameters, which I have no problem with, so I don't see any harm in keeping this functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes so far, really looking forward for this pull request as it will make OT-PHP usable for many more automatically instrumented scenarios.
src/OpenTracing/Tracer.php
Outdated
@@ -14,6 +14,39 @@ | |||
interface Tracer | |||
{ | |||
/** | |||
* @deprecated use either startActiveSpan or startManualSpan instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not yet 1.0, so I would just go and remove this instead of deprecating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you think we should use startActiveSpan
or startSpan
making the active
condition by default @beberlei?
In my opinion it makes it consistent with other language APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I just changed it.
src/OpenTracing/ActiveSpanSource.php
Outdated
* must not be changed (idempotency). | ||
* | ||
* Do not confuse it with the finish concept: | ||
* - $span->deactivate() the span is not active but still "running" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention Span#deactivate
here but this method does not exist. Should it? I don't think so, the comment must probably be adjusted to:
- $tracer->getActiveSpanSource()->deactivate($span); the span is not active anymore but still "running"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one. I just fixed it.
2dd6fda
to
0879abd
Compare
@@ -25,7 +58,26 @@ | |||
* @throws InvalidSpanOption for invalid option | |||
* @throws InvalidReferencesSet for invalid references set | |||
*/ | |||
public function startSpan($operationName, $options = []); | |||
public function startActiveSpan($operationName, $options = []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reference type between the new created active span and the current active span?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @jcchavezs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating a new activeSpan
it becomes automatically the child of the existing one: https://github.com/opentracing/opentracing-php/pull/45/files#diff-bd85a3792bd64b152939e0b1b78086e2R19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about other span relations than child_of
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixfbecker that is what startManualSpan
is for.
/** | ||
* @return ActiveSpanSource | ||
*/ | ||
public function getActiveSpanSource(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of ActiveSpanSource is not bad. Because, I implemented the same function in my company's php framework yesterday.
However, the opentracing specification only defines the references. And how to trace the reference is all business of user.
/** | ||
* Keeps track of the current active `Span`. | ||
*/ | ||
interface ActiveSpanSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we shift the ActiveSpanSource interface, why not shift a ActiveSpanSource implementation?
In my opinion, the ActiveSpanSource is only like a container. And only one implementation is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it this interface would be important to implement for event loops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcchavezs is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified this piece in #45
I need to make a summary of my consideration. @beberlei @felixfbecker @yurishkuro @tPl0ch @jcchavezs @tedsuo @jonahgeorge My Opinion
I have said these words again and again. In my opinion, the job to maintain the reference between span should leave to the user, even not the OT vendor. However, if we really like to shift this feature, there still some issue in this PR. The current issues
|
@jcchavezs Please following the OT spec. |
Unfortunately the implementation details changed and the Python PR (which is the one I based this PR on) has been closed in favour of opentracing/opentracing-python#63. I am closing this PR for now. I created an issue for this matter and I will open a new PR by this week. |
@jcchavezs the only thing that changed in the new python implementation is this weird with something as scope change, which PHP doesn't have in the language (blocks), so we should keep this PR. |
@lvht, thanks for the feedback. Please let's follow up further questions in #45
In some cases you don't want to create a scope for in-process context propagation. For example in concurrency situations, after
This is a valid concern an a good question. We discussed this a long time ago and we decided to decouple the behaviour, that is why we added this note: https://github.com/opentracing/opentracing-php/pull/45/files#diff-e01221a8e1fafbccea47b6bc2f5f9f28R33
I did a change in this direction. To be honest having the scope control inside the tracer make things more understandable. I had the opportunity to try the
You might want to add specific tags or logs to the current span and that is not possible without accessing the |
@@ -42,7 +42,7 @@ The simplest starting point is to set the global tracer. As early as possible, d | |||
|
|||
### Creating a Span given an existing Request | |||
|
|||
To start a new `Span`, you can use the `StartSpanFromContext` method. | |||
To start a new `Span`, you can use the `startActiveSpan` method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You name it startActiveSpan
here and then the example uses startManualSpan
though. Is this right?
@@ -25,7 +58,26 @@ | |||
* @throws InvalidSpanOption for invalid option | |||
* @throws InvalidReferencesSet for invalid references set | |||
*/ | |||
public function startSpan($operationName, $options = []); | |||
public function startActiveSpan($operationName, $options = []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixfbecker that is what startManualSpan
is for.
* or use the call to fastcgi_finish_request in order to not to delay the end | ||
* of the request to the client. | ||
* As an implementor, a good idea would be to use register_shutdown_function | ||
* or fastcgi_finish_request in order to not to delay the end of the request to the client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an "or" imho, since you can use both. register_shutdown_function is still waiting to finish the communication with the Webserver if you don't use fastcgi_finish_request.
Closing in favor of #47 |
This is related to the in_process_propagation.
Ping @beberlei @felixfbecker @yurishkuro @tPl0ch