Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
@cijothomas @reyang could you please have a look? Thanks. |
| /// </remarks> | ||
| public Activity SetStatus(ActivityStatusCode code, string? description = null) | ||
| { | ||
| if (code <= ActivityStatusCode.Unset || code > ActivityStatusCode.Error) |
There was a problem hiding this comment.
Would it be easier to understand as just:
if (code != ActivityStatusCode.Ok && code != ActivityStatusCode.Error)?
| { | ||
| if (code <= ActivityStatusCode.Unset || code > ActivityStatusCode.Error) | ||
| { | ||
| throw new ArgumentException(SR.Format(SR.InvalidStatusCode, code), nameof(code)); |
There was a problem hiding this comment.
ArgumentOutOfRangeExcception?
There was a problem hiding this comment.
I am not sure of that if they try Unset value. should this case considered out of range?
There was a problem hiding this comment.
What if we let them set any value they want and don't have this error condition at all?
There was a problem hiding this comment.
Don't see any reason why we want to prevent users from calling SetStatus(ActivityStatusCode.Unset). Only restriction is about Ignoring Description except for Error.
There was a problem hiding this comment.
The Question now for @noahfalk suggestion. do you think we can allow any arbitrary values? I mean is it ok to use something like (ActivityStatusCode) (-10)?
@reyang as you were the one advocating for not allowing Unset, could you please respond if you have any objection allowing Unset to be set and what you think allowing any status code even not defined ones in ActivityStatusCode?
If all agrees, I can modify the code as follows:
- Allow any status code to be set (including non defined values).
- Any code not equal
Error, I'll reset the status description tonull.
There was a problem hiding this comment.
I will refer to the spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-status.
My interpretation is that:
- the default value is
unset - instrumentation library could set it to
error, with an optional description - others (e.g. application) could set it to
ok, in this case there shouldn't be a description (so the description is set back to null)
Other cases should not happen, we can either ignore, throw exception, or fall back to a default behavior.
There was a problem hiding this comment.
I had offline discussion with @reyang and @cijothomas and here is the behavior we are going with:
- We'll allow setting all status codes. That include
Unsetor even undefined values (e.g.(ActivityStatusCode)100). - Setting any code different than
Errorwill reset the description to null. - The description can be non null only with
Errorcode. - We'll never throw exceptions from setting the status.
I'll go ahead and update the code according to that.
| { | ||
| if (code <= ActivityStatusCode.Unset || code > ActivityStatusCode.Error) | ||
| { | ||
| throw new ArgumentException(SR.Format(SR.InvalidStatusCode, code), nameof(code)); |
There was a problem hiding this comment.
FWIW, I'm still not clear on why it's deemed acceptable to transition back and forth between Ok and Error but why it's not ok to transition to Unset.
There was a problem hiding this comment.
@cijothomas @reyang may answer this better.
The specs says: Description MUST be IGNORED for StatusCode Ok & Unset values.. This means it is allowed to set Unset. Also, my current implementation is throwing in such cases. Should we just ignore the description in such cases as the specs suggest?
There was a problem hiding this comment.
If the spec says these conditions are ok, and we're adding these to support the spec, I don't know why we'd add our own artificial limitations. (At that point, you don't even need this method and could just put setters on the properties.)
There was a problem hiding this comment.
I think we still need SetStatus. The reason is we need to reset the description message to null when setting the code to something different than Error. That is what the specs is requiring.
We can still do that in properties, but would confusing if setting one property changing the other comparing to SetStatus which already take description parameter which we can define and document the behavior there.
Fixes #46689