Skip to content

Trap agent errors in DDTracingClientExec#258

Merged
realark merged 1 commit into
masterfrom
ark/httpclientwrapper_fix
Mar 13, 2018
Merged

Trap agent errors in DDTracingClientExec#258
realark merged 1 commit into
masterfrom
ark/httpclientwrapper_fix

Conversation

@realark
Copy link
Copy Markdown
Contributor

@realark realark commented Mar 12, 2018

Our HTTPClient instrumentation inserts itself into the client's exec chain to create spans. This requires the instrumentation to re-throw exceptions, but the instrumentation was not capturing internal errors which should have been caught.

This change guards against internal agent exceptions being thrown into the client.

@realark realark added tag: do not merge Do not merge changes type: bug Bug report and fix and removed tag: do not merge Do not merge changes labels Mar 12, 2018
@realark realark requested a review from tylerbenson March 12, 2018 22:18
Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Could you add some comments in the code explaining the change. It's not really clear why it was needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume networkSpan might be null here. Is that a problem?

@realark realark force-pushed the ark/httpclientwrapper_fix branch from a5acfab to c3c76c8 Compare March 12, 2018 23:33
@realark realark dismissed tylerbenson’s stale review March 13, 2018 00:34

Added some comments and a null check when setting the status code.

@realark realark merged commit 87e09d7 into master Mar 13, 2018
@realark realark deleted the ark/httpclientwrapper_fix branch March 13, 2018 01:23
@realark realark added this to the 0.5.0 milestone Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug Bug report and fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants