Add setStatus in Span

[hectorhdzg]

Adding setStatus method in Span according to spec

#212

[reyang]

I'm confused, I thought is_ok is deduced from the code?

[Oberon00]

I created open-telemetry/opentelemetry-specification#297 in the spec, but I think the is_ok getter of the code should just use canonical_code.

[reyang]

Please find my comments.

[hectorhdzg]

Thanks everyone for taking your time to review this PR, I appreciate all feedback, I tried to address all comments please let me know if you have more feedback. I added a new section in the docs for opentelemetry.trace.status as well in last update
@reyang @c24t @Oberon00 @lzchen

[c24t]

These are off by one in the generated docs since they're parsed as attribute docstrings. Since I had to make the change to check it anyway I put it in a PR for this branch: hectorhdzg#1.

[reyang]

LGTM, I've left some non-blocking comments.

[Oberon00]

AFAIK, attribute docstrings are not a thing in Python. So I guess to the interpreter these are just string expressions without any effect. Thus, and for consistency, I'd prefer using #: comments like elsewhere (

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

Lines 123 to 140 in 63f559e

	#: Default value. Indicates that the span is used internally in the application.
	INTERNAL = 0
	#: Indicates that the span describes an operation that handles a remote request.
	SERVER = 1
	#: Indicates that the span describes a request to some remote service.
	CLIENT = 2
	#: Indicates that the span describes a producer sending a message to a
	#: broker. Unlike client and server, there is usually no direct critical
	#: path latency relationship between producer and consumer spans.
	PRODUCER = 3
	#: Indicates that the span describes consumer receiving a message from a
	#: broker. Unlike client and server, there is usually no direct critical
	#: path latency relationship between producer and consumer spans.
	CONSUMER = 4

) to these "docstrings".

[c24t]

To the interpreter, but not to sphinx! Sphinx handles both "docstring"-style strings like this and regular comments.

I don't have a strong preference here, either comment style works for me as long as the comments and members line up in the generated docs.

@Oberon00 what's with the : in #:?

[Oberon00]

Only minor comments. Looking mostly good to me now! 👍

[Oberon00]

Suggested change

	canonical_code: "StatusCanonicalCode" = StatusCanonicalCode.OK,
	canonical_code: StatusCanonicalCode,

• No quotes for types if not required.
• I'd remove the default argument for the code: If you explicitly construct a status, most of the time it won't be OK.

[hectorhdzg]

I added the quotes for consistency with other files, I thought it was some kind of coding standard used here, if this is an issue maybe need to be addressed everywhere in the code.

Related to default status, spec says default is OK, that is the reason I added the default parameter
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#set-status

[c24t]

I added the quotes for consistency with other files

Very much a nitpick, but I think it actually is a good idea to quote all non-standard types in these annotations. This way we can move classes around within modules without mypy complaining about forward references. One less mypy annoyance to deal with this way.

[Oberon00]

@hectorhdzg: Could you please comment on why you made them properties instead of simply public attributes?

[Oberon00]

We could add a constant of type Status named OK for the Status(OK, None) object in the status module and use it here to save object allocations.

[hectorhdzg]

Why adding a constant if this is already the default Status?, is this some kind of pattern? sound a little odd for me

[c24t]

It's a small optimization to avoid creating a new "OK" status with each new span when they could all use the same instance instead.

OK statuses are all alike, every not-OK status is not-OK in its own way. So we may as well use a single instance for the OK status.

[c24t]

No more blocking comments from me, thanks @hectorhdzg!

[c24t]

I talked with @hectorhdzg and we think this is ready to merge. We can follow up on lingering comments in other PRs.