Add `validateStatus` option to HTTP plugin #301

timrogers · 2018-10-04T09:19:22Z

The OpenTracing spec states that a span should be tagged as an error "if and only if the application considers the operation represented by the Span to have failed".

The dd-trace-js HTTP plugin allows you to automatically instrument outgoing HTTP requests.

Currently, it only tags a span as an error when the request emits an error event, which only happens in the case of "hard failures" like a timeout or connection problem.

dd-trace-js/src/plugins/http.js

Lines 59 to 67 in 34c1685

    
           req.on('error', err => { 
        
             span.addTags({ 
        
               'error.type': err.name, 
        
               'error.msg': err.message, 
        
               'error.stack': err.stack 
        
             }) 
        
             span.finish() 
        
           })

This means that requests that return responses never are considered errors, even if a 4XX or 5XX response is received.

Having spans tagged as errors is useful, because it means they are flagged as such in the Datadog APM interface, useful for reporting.

This commit adds support for a new validateStatus option for the HTTP plugin which allows the user to supply a validateStatus function which, when a response is received, is passed the response status and expected to return false if the response should be considered an error, or true if not. If it returns false, the span will be marked as an error.

By default, we will consider requests that return 4XX responses to be errors, since a 4XX indicates a failure which is within the user's control.

Fixes #297.

rochdev

After thinking about this a bit more and discussing with the team, 4xx responses should also be errors by default since it means the error is from the client. 5xx should still not be errors by default since the error comes from the server.

rochdev · 2018-10-04T13:24:20Z

docs/API.md

@@ -216,6 +216,7 @@ query HelloWorld {
 |------------------|------------------|----------------------------------------|
 | service          | http-client      | The service name for this integration. |
 | splitByDomain    | false            | Use the remote endpoint host as the service name instead of the default. |
+| validateStatus   | `code => true`   | Callback function to determine if an HTTP response should be recorded as an error. It should take a status code as its only parameter and return `true` for success or `false` for errors. Requests where no response is received (e.g. connection failures, timeouts) will always be considered to be errors.


Requests where no response is received (e.g. connection failures, timeouts) will always be considered to be errors.

Since this configuration focuses on the status code, network errors are our of its scope, so this sentence should be removed.

Happy to remove this, but I think this is a useful point of clarification to make it clear that this doesn't configure how "no response" is handled.

I think it's redundant since validateStatus implies a response from the remote endpoint.

Fair enough! I'll ✂️ this.

Perhaps a better place for this would be in some more general documentation for the HTTP plugin? I've found that I need to delve into the code to answer questions about the specifics.

Definitely feel free to add any information you think is relevant to the top of the plugin documentation :)

I'll do this in a separate PR.

rochdev · 2018-10-04T13:24:59Z

src/plugins/http.js

@@ -53,6 +53,12 @@ function patch (http, methodName, tracer, config) {
      req.on('response', res => {
        span.setTag(Tags.HTTP_STATUS_CODE, res.statusCode)

+        if (config.validateStatus && !config.validateStatus(res.statusCode)) {
+          span.addTags({


Small nit: use setTag to add a single tag.

timrogers · 2018-10-04T15:03:14Z

@rochdev Thanks! I'll make those tweaks later.

The [OpenTracing spec][1] states that a span should be tagged as an error "if and only if the application considers the operation represented by the Span to have failed". The `dd-trace-js` HTTP plugin allows you to automatically instrument outgoing HTTP requests. Currently, it only tags a span as an error when the request emits an `error` event, which only happens in the case of "hard failures" like a timeout or connection problem. https://github.com/DataDog/dd-trace-js/blob/34c168555f44a961a61ede95a39341d7c527701a/src/plugins/http.js#L59-L67 This means that requests that return responses never are considered errors, even if a 4XX or 5XX response is received. Having spans tagged as errors is useful, because it means they are flagged as such in the Datadog APM interface, useful for reporting. This commit adds support for a new `validateStatus` option for the HTTP plugin which allows the user to supply a `validateStatus` function which, when a response is received, is passed the response status and expected to return `false` if the response should be considered an error, or `true` if not. If it returns `false`, the span will be marked as an error. By default, we will consider requests that return `4XX` responses to be errors, since a `4XX` indicates a failure which is within the user's control. Fixes DataDog#297. [1]: https://github.com/opentracing/specification/blob/master/semantic_conventions.md

timrogers · 2018-10-06T20:01:41Z

@rochdev Updated!

johnnydimas · 2018-11-22T20:08:56Z

Hi @rochdev! Me again :)

I updated to 0.7.0 - happy this has been released. :)

However, I'm not getting my 4xx status codes marked as errors in APM. From this thread it sounded like they would be considered errors by default, but maybe I am misunderstanding something. Do I need to add the valiadateStatus to my code to mark 4xx as errors?

I am using node 10.13.0 with express for my routing.

Thanks so much!

rochdev · 2018-11-22T20:15:19Z

@johnnydimas It actually depends since the http plugin is used for both servers and clients.

For HTTP servers:

Network errors are considered errors
5xx errors are considered errors
Other status codes are considered OK

For HTTP clients:

Network errors are considered errors
4xx errors are considered errors
Other status codes are considered OK

Right now the validateStatus allows you to configure this behavior, but unfortunately right now it will configure both the client and server at the same time.

If you need to configure them individually, please let me know and I can provide a workaround.

johnnydimas · 2018-11-23T10:45:11Z

@rochdev Thanks so much for the help, I was able to get it working now!

rochdev requested changes Oct 4, 2018

View reviewed changes

rochdev added enhancement New feature or request community integrations labels Oct 4, 2018

timrogers force-pushed the http-validate-status branch from dac46cd to b2bfaff Compare October 6, 2018 20:01

Merge branch 'master' into http-validate-status

03e21c3

rochdev added this to the 0.7.0 milestone Oct 9, 2018

rochdev approved these changes Oct 9, 2018

View reviewed changes

Merge branch 'master' into http-validate-status

8539730

rochdev changed the base branch from master to v0.7.0 October 23, 2018 14:32

rochdev merged commit 47c945c into DataDog:v0.7.0 Oct 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `validateStatus` option to HTTP plugin #301

Add `validateStatus` option to HTTP plugin #301

timrogers commented Oct 4, 2018 •

edited

rochdev left a comment

rochdev Oct 4, 2018

timrogers Oct 4, 2018

rochdev Oct 4, 2018

timrogers Oct 4, 2018

rochdev Oct 4, 2018

timrogers Oct 6, 2018

rochdev Oct 4, 2018

timrogers commented Oct 4, 2018

timrogers commented Oct 6, 2018

johnnydimas commented Nov 22, 2018 •

edited

rochdev commented Nov 22, 2018

johnnydimas commented Nov 23, 2018

	req.on('error', err => {
	span.addTags({
	'error.type': err.name,
	'error.msg': err.message,
	'error.stack': err.stack
	})

	span.finish()
	})

Add validateStatus option to HTTP plugin #301

Add validateStatus option to HTTP plugin #301

Conversation

timrogers commented Oct 4, 2018 • edited

rochdev left a comment

Choose a reason for hiding this comment

rochdev Oct 4, 2018

Choose a reason for hiding this comment

timrogers Oct 4, 2018

Choose a reason for hiding this comment

rochdev Oct 4, 2018

Choose a reason for hiding this comment

timrogers Oct 4, 2018

Choose a reason for hiding this comment

rochdev Oct 4, 2018

Choose a reason for hiding this comment

timrogers Oct 6, 2018

Choose a reason for hiding this comment

rochdev Oct 4, 2018

Choose a reason for hiding this comment

timrogers commented Oct 4, 2018

timrogers commented Oct 6, 2018

johnnydimas commented Nov 22, 2018 • edited

rochdev commented Nov 22, 2018

johnnydimas commented Nov 23, 2018

Add `validateStatus` option to HTTP plugin #301

Add `validateStatus` option to HTTP plugin #301

timrogers commented Oct 4, 2018 •

edited

johnnydimas commented Nov 22, 2018 •

edited