Skip to content
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 TS example for recording exceptions #5907

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Conversation

Grunet
Copy link
Contributor

@Grunet Grunet commented Jan 10, 2025

Resolves #5805

I tested in the Typescript playground that this fixes the type checking issue.

I have not checked that this fully functionally works (is there an existing otel-js playground for that? If not I can probably find an online editor playground to setup in. Just lmk)

@Grunet Grunet requested a review from a team as a code owner January 10, 2025 03:08
@opentelemetrybot opentelemetrybot requested a review from a team January 10, 2025 03:09
@theletterf
Copy link
Member

@open-telemetry/javascript-approvers please have a look — thanks!

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks 🙂

@Grunet I'm not aware of a playground to try things like this, but I checked that locally when I suggested it over at #5085. That's also the way we do it in some OTel instrumentations so I'm confident that this is the correct way to do it. 👍

Functionally, there's a difference to what it did before as it won't record anything that's not an error (recordException would technically allow for custom types that roughly match Error, and strings too), but I think this check covers the vast majority of cases. If users need to record something other than Error then it's still possible to check up on the type-defintion of Span#recordException() but I don't think it needs to be spelled out in the docs :)

@svrnm
Copy link
Member

svrnm commented Jan 10, 2025

thanks @Grunet ! For testing/making sure that the code works we want to roll out a "code excerpt" solution long term, until then we appreciate if people test their work locally and provide it afterwards.

@cartermp
Copy link
Contributor

I tested this and it looks good. Thanks!

@cartermp cartermp added this pull request to the merge queue Jan 10, 2025
Merged via the queue into open-telemetry:main with commit 5f6ef6e Jan 10, 2025
17 checks passed
meswapnilk pushed a commit to meswapnilk/opentelemetry.io that referenced this pull request Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Recording Exceptions Example for Typescript Doesn't Work Out of the Box
5 participants