span: implement update_name method #112
Conversation
mauriciovasquezbernal
requested review from
a-feld,
c24t,
carlosalberto,
lzchen,
Oberon00,
reyang and
toumorokoshi
as code owners
mauriciovasquezbernal
force-pushed
the
mauricio/add_span_update_name
branch
from
ad40d5a to
0c59ba3
Compare
| @@ -261,6 +261,11 @@ def end(self): | |||
| if self.end_time is None: | |||
| self.end_time = util.time_ns() | |||
|
|
|||
| def update_name(self: 'Span', name: str) -> None: | |||
| if not name: | |||
| raise ValueError("Name cannot be empty") | |||
There was a problem hiding this comment.
Is an empty string for name actually illegal?
There was a problem hiding this comment.
There is nothing about it the specification. I think we could allow it, I got confused by a check in the Java implementation but this is actually checking if the string is not null, not if this is empty.
mauriciovasquezbernal
force-pushed
the
mauricio/add_span_update_name
branch
from
0c59ba3 to
c62b413
Compare
|
Let's wait for a day to get comments before merge. |
| Upon this update, any sampling behavior based on Span name will depend | ||
| on the implementation. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Shouldn't it raise a NotImplementedError?
| raise NotImplementedError |
There was a problem hiding this comment.
I don't know what is the preferred way to implement interfaces in python without using ABC. So far none of the methods in the opentelemetry.Span class raise exceptions. Should they?
This PR is just following the current style, maybe this discussion should be moved to #66.
| This will override the name provided via :func:`Tracer.create_span` | ||
| or :func:`Tracer.start_span`. | ||
|
|
||
| Upon this update, any sampling behavior based on Span name will depend |
There was a problem hiding this comment.
| Upon this update, any sampling behavior based on Span name will depend | |
| After this update, any sampling behavior based on Span name will depend |
There was a problem hiding this comment.
I just took this sentence from the specification:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#updatename
| @@ -261,6 +261,9 @@ def end(self): | |||
| if self.end_time is None: | |||
| self.end_time = util.time_ns() | |||
|
|
|||
| def update_name(self: 'Span', name: str) -> None: | |||
There was a problem hiding this comment.
| def update_name(self: 'Span', name: str) -> None: | |
| def update_name(self: Span, name: str) -> None: |
Not sure if that also works in the API package, but here it should definitely work without quotes.
There was a problem hiding this comment.
Let's go a step back first, should we really annotate the self member?, In the examples in https://www.python.org/dev/peps/pep-0484 it is not annotated.
There was a problem hiding this comment.
👍 You are right AFAIK, self is implicitly annotated with the containing class.
|
@Oberon00 I am new here I don't have a strong opinion whether we should "pythonize" the API or not. My only comment is that if we use a member we could put all the name related thing directly on the API without touching the SDK. |
mauriciovasquezbernal
force-pushed
the
mauricio/add_span_update_name
branch
from
c62b413 to
c789415
Compare
|
On the pythonization: I'd rather stay with the explicit name ( If we decide to go with the pythonic route, then we need to expose the |
Resolve open-telemetry#111 Signed-off-by: Olivier Albertini <olivier.albertini@montreal.ca>
Fixes: #99