Parent is now always passed in via Context, intead of Span or SpanContext #1146

codeboten · 2020-09-22T23:52:58Z

Description

As per the spec change open-telemetry/opentelemetry-specification#875, the parent must always be passed via a Context.

Fixes #1139

Type of change

Please delete options that are not relevant.

Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been updated
Documentation has been updated

…text

toumorokoshi

one non-blocking comment on variable naming. I think it's probably one that needs to be addressed, but don't want to stop progress if I don't catch up to this PR.

toumorokoshi · 2020-10-03T05:21:20Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py


        if parent_context is not None and not isinstance(
            parent_context, trace_api.SpanContext
        ):
-            raise TypeError("parent must be a Span, SpanContext or None.")
+            raise TypeError("parent_context must be a SpanContext or None.")


I guess this type validation is still valuable. But practically speaking, this would only be an issue if someone directly manipulates the context, and doesn't use the API functions.

Is that a common enough case to even catch here?

Alternatively I suppose this is cheap enough.

toumorokoshi · 2020-10-03T05:25:49Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -228,7 +226,7 @@ class Tracer(abc.ABC):
    def start_span(
        self,
        name: str,
-        parent: ParentSpan = CURRENT_SPAN,
+        parent: typing.Optional[Context] = None,


I'm wondering if the variable is worth renaming. "parent" was vague to begin with, and if you look at any descriptions of how tracing works, one would immediately think "parent" refers to the span (since you specifically call the span a child of the parent span).

This would also be useful to rename since new API users will get exceptions when their existing code passes an argument as parent. Rather than get a new slightly confusing error ("why is the API now rejecting this previously valid value?"), they will get an argument not supported error, and look up the docs to see the variable changed.

parent_context is the first thing that comes to mind, but I worry a user would get it confused with the parent SpanContext.

@toumorokoshi yeah I had a similar thought around the name. Looking at some of the other implementations, it appears parent and parent_context are the current names used. I'm happy to change it to parent_context as it's at least a bit more meaningful than parent. The alternative i can think of would be to just call it context

Either sounds great!

I'd say context is the better name. A Context object passed in this argument may or may not contain a parent Span, making the name parent_context less accurate and context_with_or_without_the_parent_span is a bit long.

Thanks for the feedback, went with context.

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

lzchen · 2020-10-06T15:05:17Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

-        parent_context = parent
-        if isinstance(parent_context, trace_api.Span):
-            parent_context = parent.get_context()
+        parent_context = trace_api.get_current_span(parent).get_context()


Now that there is the concept of a Context being passed into span start, I'm wondering if it is valuable to rename span.get_context() to span.get_span_context() to avoid confusion. I know that the Context isn't actually a PART of Span, it probably is only here (during start and use) that it would be confusing. Thoughts?

And maybe even renaming the variable from parent_context to parent_span_context.

I would +1 on "get_span_context", although with open-telemetry/opentelemetry-specification#1033 that might not even be necessary.

I renamed get_context to get_span_context and parent_context to parent_span_context where it made sense. I tested the water with renaming context in the Span to span_context and it turned out to be pretty messy. Not sure if it's worth the rename on that one. Thoughts @lzchen & @toumorokoshi?

I think that's fine, I don't know if I have a strong opinion on the attribute name on the span.

And I'm completely comfortable with taking these PRs piecemeal too.

ocelotl

Just some minor comments

ocelotl · 2020-10-06T16:10:49Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -228,7 +226,7 @@ class Tracer(abc.ABC):
    def start_span(
        self,
        name: str,
-        parent: ParentSpan = CURRENT_SPAN,
+        parent: typing.Optional[Context] = None,


I'd say context is the better name. A Context object passed in this argument may or may not contain a parent Span, making the name parent_context less accurate and context_with_or_without_the_parent_span is a bit long.

opentelemetry-api/src/opentelemetry/trace/__init__.py

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

parent is now always passed in via Context, intead of Span or SpanCon…

4e87e13

…text

codeboten changed the title ~~WIP: Parent is now always passed in via Context, intead of Span or SpanCon…~~ WIP: Parent is now always passed in via Context, intead of Span or SpanContext Sep 22, 2020

codeboten added the release:required-for-ga To be resolved before GA release label Sep 24, 2020

codeboten self-assigned this Sep 24, 2020

lzchen mentioned this pull request Sep 29, 2020

docs: updating trace API docstrings #1170

Merged

3 tasks

alrex added 3 commits September 30, 2020 17:23

Merge branch 'master' into parent-context

5180478

fix tests

9e707e3

fix one more test

3493bce

Alex Boten added 8 commits October 1, 2020 15:11

fix test

34b6cc1

Merge branch 'master' into parent-context

3e2ffab

fix lint

eceae0f

fix more tests

865891c

fix import path

1acb6db

fix instrumentation tests

01502ca

fix shim

42c574f

updating docs/changelog

f82bd23

codeboten changed the title ~~WIP: Parent is now always passed in via Context, intead of Span or SpanContext~~ Parent is now always passed in via Context, intead of Span or SpanContext Oct 2, 2020

codeboten marked this pull request as ready for review October 2, 2020 04:57

codeboten requested a review from a team October 2, 2020 04:57

toumorokoshi approved these changes Oct 3, 2020

View reviewed changes

lzchen reviewed Oct 6, 2020

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Show resolved Hide resolved

lzchen reviewed Oct 6, 2020

View reviewed changes

lzchen approved these changes Oct 6, 2020

View reviewed changes

alrex added 4 commits October 6, 2020 09:03

Merge branch 'master' into parent-context

76107e5

rename parent parameter to context

b388cd8

fix celery instrumentation

3114a13

fix lint

a2dfae9

ocelotl approved these changes Oct 6, 2020

View reviewed changes

alrex and others added 9 commits October 6, 2020 13:17

Apply suggestions from code review

594f9f8

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

Merge branch 'master' into parent-context

aa1e329

rename get_context to get_span_context

a451c4f

rename parent_context to parent_span_context where appropriate

c6f5289

fix test

2585e3f

fix lint

6a910fa

Merge branch 'master' into parent-context

3b49754

fix lint

bf512da

Merge branch 'master' into parent-context

77fba4a

codeboten merged commit 0e852ea into open-telemetry:master Oct 8, 2020

codeboten deleted the parent-context branch October 8, 2020 15:39

codeboten mentioned this pull request Oct 14, 2020

Add coding examples for context #1165

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parent is now always passed in via Context, intead of Span or SpanContext #1146

Parent is now always passed in via Context, intead of Span or SpanContext #1146

codeboten commented Sep 22, 2020 •

edited

Loading

toumorokoshi left a comment

toumorokoshi Oct 3, 2020

toumorokoshi Oct 3, 2020

codeboten Oct 6, 2020

toumorokoshi Oct 6, 2020

ocelotl Oct 6, 2020

codeboten Oct 6, 2020

lzchen Oct 6, 2020 •

edited

Loading

lzchen Oct 6, 2020

toumorokoshi Oct 7, 2020

codeboten Oct 7, 2020

toumorokoshi Oct 8, 2020

ocelotl left a comment

ocelotl Oct 6, 2020

Parent is now always passed in via Context, intead of Span or SpanContext #1146

Parent is now always passed in via Context, intead of Span or SpanContext #1146

Conversation

codeboten commented Sep 22, 2020 • edited Loading

Description

Type of change

Checklist:

toumorokoshi 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

lzchen Oct 6, 2020 • edited Loading

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

ocelotl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten commented Sep 22, 2020 •

edited

Loading

lzchen Oct 6, 2020 •

edited

Loading