Set the scope_manager RFC to Test. #122

carlosalberto · 2018-07-05T16:27:35Z

Initial take on the ScopeManager RFC.

Notes:

Used 'thread or execution unit' in order to abstract things such as coroutines.
Mentioned Scopes are not thread-safe.
Mentioned the null the value (it hadn't been mentioned before in the Specification).
Light take on the recent start_active_span() vs start_active_scope() debate.
Used ScopeManager.active() (as opposed to ScopeManager.activeSpan()). Prone to change based on further discussion.

Please provide feedback ;)

@tedsuo @palazzem @pavolloffay @pglombardo @yurishkuro @jpkrohling @indrekj @mwear @felixbarny @cwe1ss @adriancole

tylerbenson · 2018-07-05T22:28:43Z

specification.md

+
+There should be no parameters.
+
+**Returns** a `Scope` instance containing the active `Span`, or else `null` if there's none for the current thread or execution unit.


This says it returns a Scope, but the title implies returning a Span.

Oh yes, think we should tune that in.

Yes I think it should be Scope, regardless the name we have in our implementations.

tedsuo

Generally LGTM

palazzem · 2018-07-13T15:46:03Z

specification.md

+
+There should be no parameters.
+
+**Returns** the used `ScopeManager` instance to set and retrieve the active `Span`. The mentioned active instance is additinaly used by default as the implicit parent for newly created `Span`s, in case no **references** were provided.


"the active Span" -> "the active Scope"? or in general I think it should be clear that the ScopeManager retrieves a Scope that contains the active Span. Thoughts?

Yeah, I think it makes more sense to make it clear at all moments that Scope objects are being returned/used.

palazzem · 2018-07-13T15:55:40Z

specification.md

+
+This operation does the same as **Start a new `Span`**, in addition to setting the newly created `Span` as the active instance for the current thread or execution unit through the contained `ScopeManager` instance.
+
+It is **highly** encouraged that the name for this operation includes a `Scope` sufix, in order to make clear the type of the returned instance.


Scope sufix -> Scope suffix?

name of the method or name of the span?

Oh, method name ;) Will clarify that.

palazzem · 2018-07-13T16:07:49Z

specification.md

@@ -250,6 +277,46 @@ In OpenTracing we force `SpanContext` instances to be **immutable** in order to

 This is modeled in different ways depending on the language, but semantically the caller should be able to efficiently iterate through all baggage items in one pass given a `SpanContext` instance.

+### `ScopeManager`
+
+The `ScopeManager` interface sets and retrieves the active `Span`, working along the `Scope` container interface.


Related to my previous comment. Here I guess the behavior is clear, but I think we may need to change something in the other paragraph.

pavolloffay · 2018-07-16T07:58:22Z

specification.md

+
+This operation does the same as **Start a new `Span`**, in addition to setting the newly created `Span` as the active instance for the current thread or execution unit through the contained `ScopeManager` instance.
+
+It is **highly** encouraged that the name for this operation includes a `Scope` sufix, in order to make clear the type of the returned instance.


name of the method or name of the span?

pavolloffay · 2018-07-16T08:05:56Z

specification.md

+
+There should be no parameters.
+
+**Returns** a `Scope` instance containing the active `Span`, or else `null` if there's none for the current thread or execution unit.


It would be good to resolve opentracing/opentracing-java#267 before adding this to the spec

I'd say it's more a related effort - as part of this very PR, we need to discuss what is described in #267 (as well as opentracing/opentracing-ruby#36).

But yes, in practical terms, we need to figure those two issues above before finally merging this PR 😉

indrekj · 2018-10-11T17:28:53Z

specification.md

+
+### `Scope`
+
+The `Scope` interface acts as a simple container for the active `Span`,and it not guaranteed to be thread-safe. It has the following capabilities:


and it not guaranteed

indrekj · 2018-10-11T17:46:03Z

Maybe not directly related to this PR but anyway:

The doc explains the behavior of Scope / ScopeManager, but I feel like it's missing the why: Why is scope / scope manager needed? Why wouldn't Tracer#activate(span) :: span work instead? I've been experimenting with opentracing spec for elixir and the scope manager part is confusing to me atm. There's likely an answer in some old PR but it's hard to find it.

I've implemented the scope manager part in ruby zipkin OT and we've used it in production for some time now. So far I haven't encountered a use case where a simple activate(span) :: span & deactivate() without Scope container wouldn't work as well.

carlosalberto · 2018-11-14T15:27:26Z

Hey @indrekj @pavolloffay @palazzem @tylerbenson @tedsuo

I've updated the document with minor corrections and updating it based on the latest OT Java iteration (making some operations optional). Let me know.

carlosalberto · 2018-11-14T16:36:51Z

Hey @indrekj

Why is scope / scope manager needed?

We essentially the concept has been to separate the active Span handling from the Tracer, API-wise. Now, as we have iterated, the idea has been to provide shortcuts for the most common operations - but stuff is still expected to be in the ScopeManager instance ;)

carlosalberto · 2018-12-07T03:58:09Z

Hey all!

I think I've updated all the requested bits of this PR, and I think it's ready to go. If you have some time over the next days, let me know - else I will be merging it soon (remember it's still an ongoing effort and can be changed/updated later).

Jamim · 2018-12-07T15:32:08Z

rfc/scope_manager.md

@@ -1,6 +1,6 @@
 # Scope Manager

-**Current State:** Draft  
+**Current State:** Test


Hello @carlosalberto,
I think you should not remove two ending spaces here.
They are required to split lines.

When you do want to insert a <br /> break tag using Markdown, you end a line with two or more spaces, then type return.

Currently, you have Current State and Author on the same line.

Wouldn't a blank line between them do the same? Linters do not like trailing spaces (they even show up as red in git diff).

Alternatively, in this case it's appropriate to us bullet points for the metadata.

Wouldn't a blank line between them do the same?

No, it wouldn't. The result won't be the same.

Linters do not like trailing spaces

I know, but it's an issue of those linters and git.
It's not a Markdown issue.

Set the scope_manager RFC to Test.

d75ed3a

tylerbenson reviewed Jul 5, 2018

View reviewed changes

carlosalberto mentioned this pull request Jul 10, 2018

Rename start_active_span() to start_active_scope(). opentracing/opentracing-python#89

Open

tedsuo self-requested a review July 13, 2018 00:35

tedsuo approved these changes Jul 13, 2018

View reviewed changes

palazzem reviewed Jul 13, 2018

View reviewed changes

pavolloffay reviewed Jul 16, 2018

View reviewed changes

carlosalberto mentioned this pull request Aug 22, 2018

Scope Manager should use continuation passing #126

Open

indrekj reviewed Oct 11, 2018

View reviewed changes

carlosalberto mentioned this pull request Oct 18, 2018

Allow clearing the current Scope. opentracing/opentracing-java#313

Open

carlosalberto added 2 commits November 13, 2018 18:47

Minor corrections.

ce00608

Update the RFC with our latest OT Java iteration.

434e676

Jamim reviewed Dec 7, 2018

View reviewed changes

yurishkuro closed this May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set the scope_manager RFC to Test. #122

Set the scope_manager RFC to Test. #122

carlosalberto commented Jul 5, 2018

tylerbenson Jul 5, 2018

carlosalberto Jul 9, 2018

palazzem Jul 13, 2018

tedsuo left a comment

palazzem Jul 13, 2018

carlosalberto Jul 16, 2018

palazzem Jul 13, 2018

pavolloffay Jul 16, 2018

carlosalberto Jul 16, 2018

palazzem Jul 13, 2018

pavolloffay Jul 16, 2018

pavolloffay Jul 16, 2018

carlosalberto Jul 16, 2018

indrekj Oct 11, 2018

indrekj commented Oct 11, 2018

carlosalberto commented Nov 14, 2018

carlosalberto commented Nov 14, 2018

carlosalberto commented Dec 7, 2018

Jamim Dec 7, 2018

yurishkuro Dec 7, 2018

Jamim Dec 7, 2018

Jamim Dec 7, 2018


		There should be no parameters.

		Returns a `Scope` instance containing the active `Span`, or else `null` if there's none for the current thread or execution unit.


		There should be no parameters.

		Returns the used `ScopeManager` instance to set and retrieve the active `Span`. The mentioned active instance is additinaly used by default as the implicit parent for newly created `Span`s, in case no references were provided.


		This operation does the same as Start a new `Span`, in addition to setting the newly created `Span` as the active instance for the current thread or execution unit through the contained `ScopeManager` instance.

		It is highly encouraged that the name for this operation includes a `Scope` sufix, in order to make clear the type of the returned instance.


		### `Scope`

		The `Scope` interface acts as a simple container for the active `Span`,and it not guaranteed to be thread-safe. It has the following capabilities:

Set the scope_manager RFC to Test. #122

Set the scope_manager RFC to Test. #122

Conversation

carlosalberto commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedsuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indrekj commented Oct 11, 2018

carlosalberto commented Nov 14, 2018

carlosalberto commented Nov 14, 2018

carlosalberto commented Dec 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment