Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add setStatus in Span #213
Add setStatus in Span #213
Changes from 34 commits
b837c9f
a12fe5d
276ecb4
2b90351
eccef1a
a187fec
a43c980
8dfb44e
77d3649
f3af20f
1229bc7
e54a053
174ac90
2015944
437b5a5
8ae0532
798551b
35a93d4
db45017
7fe57a1
57ebc24
27df21b
4ccc142
d08c3db
5c39740
7265b92
98960ef
a714d20
ac7cf0d
1863412
27ba6a4
5e17bb7
d8f8c26
673b5be
c5e71eb
ce3552b
04709bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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
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.
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#:
?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.
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 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
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.
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.
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.
Why make these properties instead of just exposing the attributes?
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.
@Oberon00 commented before related this, using properties would be more pythonic, I'm fine with any approach
#213 (comment)
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 agree they shouldn't be getters too, I just don't see the point of exposing existing attrs under a different name.
I think it'd be fine to just expose
code
anddesc
, but if you want to hide them behind attributes I think you should change the underlying attrs to_code
and_desc
(or more conventionally:_canonical_code
,_description
).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.
@hectorhdzg: Could you please comment on why you made them properties instead of simply public attributes?
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.
is_ok require a special getter because is doing an operation to get the value, I guess other two could be just simple attributes. I felt is just cleaner to have them as properties as well, I can update if there are strong opinions about this
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.
No strong opinion as far as I'm concerned, it's just surprising to see properties wrap public attributes like this. We have both properties and bare attributes in the API now, and don't really have consistent reasons for using one over the other.
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 could add a constant of type
Status
namedOK
for theStatus(OK, None)
object in the status module and use it here to save object allocations.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.
Why adding a constant if this is already the default Status?, is this some kind of pattern? sound a little odd for me
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.
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.