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: Add status exception on Span.recordException #1490

Closed
wants to merge 11 commits into from
Closed

fix: Add status exception on Span.recordException #1490

wants to merge 11 commits into from

Conversation

jufab
Copy link
Contributor

@jufab jufab commented Sep 3, 2020

Which problem is this PR solving?

  • Span.recordException not set status

Short description of the changes

  • Add status on recordException method

@jufab jufab changed the title fix: Add status exception fix: Add status exception on Span.recordException Sep 3, 2020
@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #1490 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1490   +/-   ##
=======================================
  Coverage   93.82%   93.82%           
=======================================
  Files         154      154           
  Lines        4762     4763    +1     
  Branches      951      951           
=======================================
+ Hits         4468     4469    +1     
  Misses        294      294           
Impacted Files Coverage Δ
packages/opentelemetry-tracing/src/Span.ts 100.00% <100.00%> (ø)

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

please fix the lint errors (npm run lint:fix), other than that lgtm

@dyladan
Copy link
Member

dyladan commented Sep 3, 2020

I just looked back at the spec and it doesn't say anything about setting status when recordException is called.

@jufab
Copy link
Contributor Author

jufab commented Sep 3, 2020

I just looked back at the spec and it doesn't say anything about setting status when recordException is called.

@dyladan you right and i read this one : there is nothing about status.
But, when i try only with this spec, there's no change in backend collector (like jaeger)

noerror

but with a status code (13 for INTERNAL but maybe not the good one...)

error

So i think status code is important for an exception : that's why i do this PR.

@obecny no prob, i try to fix it.

@vmarchaud
Copy link
Member

The span.Status API should be removed from the spec fairly soon (open-telemetry/oteps#134), do we want to merge this since it will be removed anyway ?

@jufab
Copy link
Contributor Author

jufab commented Sep 4, 2020

Perhaps not a removal but a change?

open-telemetry/opentelemetry-specification#706 (comment)

merging it now, maybe, it's not a good idea...

@dyladan
Copy link
Member

dyladan commented Sep 4, 2020

An exception does not always mean the request failed. It is entirely possible to have a recorded exception which is caught and handled successfully. With span.status being removed or changed soon, I believe we should close this PR.

@jufab
Copy link
Contributor Author

jufab commented Sep 4, 2020

Ok, i close it ;)

@jufab jufab closed this Sep 4, 2020
@jufab jufab deleted the add-status-exception branch September 4, 2020 21:53
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants