-
Notifications
You must be signed in to change notification settings - Fork 767
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
fix(sdk-trace-base): avoid keeping non-string status.message on Span#setStatus() #4999
base: main
Are you sure you want to change the base?
fix(sdk-trace-base): avoid keeping non-string status.message on Span#setStatus() #4999
Conversation
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.
Left a comment but I'm not actually sure what the correct behavior should be. LMK what you think
if (this.status.message != null && typeof status.message !== 'string') { | ||
diag.warn( | ||
`Dropping invalid status.message of type '${typeof status.message}', expected 'string'` | ||
); | ||
delete this.status.message; | ||
} |
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 dropping the right behavior? If something is not nullish and also not a string, it may still be useful. Also, the user may be surprised to see nothing here. Maybe casting to string would be better?
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've been going back and forth on this as well - I've ended up with this approach as dropping it here gives us the opportunity to add casting as a string
later as a feature.
I'm thinking that if we can get away with not doing any magic - then that's better. It will keep complexity lower, but also we discourage users from breaking the type-contract with the API.
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'm thinking that if we can get away with not doing any magic - then that's better. It will keep complexity lower, but also we discourage users from breaking the type-contract with the API.
I have to admit that I though about doing "magic" but I agree with you. If the API does magic I worry the users would ask for more of it.
if (this.status.message != null && typeof status.message !== 'string') { | ||
diag.warn( | ||
`Dropping invalid status.message of type '${typeof status.message}', expected 'string'` | ||
); | ||
delete this.status.message; | ||
} |
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'm thinking that if we can get away with not doing any magic - then that's better. It will keep complexity lower, but also we discourage users from breaking the type-contract with the API.
I have to admit that I though about doing "magic" but I agree with you. If the API does magic I worry the users would ask for more of it.
Which problem is this PR solving?
See #4998,
Span#setStatus()
is commonly used in catch blocks to set the span statusSpanStatusCode.ERROR
. While doing so, it's an easy mistake to make to also pass theError
as themessage
, which expects a string, as TypeScript will no complain as you assignany
tostring
.Therefore this PR adds a validation step to ensure we don't pass on the incorrect type downstream, which can cause OTLP/JSON export requests to be rejected.
Fixes #4998
See also renovatebot/renovate#31459
Type of change
How Has This Been Tested?