Skip to content

Commit

Permalink
fix(instrumentation/https): removed unnecessary instrumentation, http…
Browse files Browse the repository at this point in the history
…s uses http in the background
  • Loading branch information
Peter Marton committed Jul 12, 2017
1 parent d66acb1 commit 79ef8fb
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 76 deletions.
22 changes: 11 additions & 11 deletions src/instrumentation/httpClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,7 @@ function extractUrl (options) {
})
}

function unpatchHttp (http) {
shimmer.unwrap(http, 'request')

if (semver.satisfies(process.version, '>=8.0.0')) {
shimmer.unwrap(http, 'get')
}
}

function patchHttp (http, tracer) {
function patch (http, tracer) {
shimmer.wrap(http, 'request', (request) => makeRequestTrace(request))

if (semver.satisfies(process.version, '>=8.0.0')) {
Expand Down Expand Up @@ -99,9 +91,17 @@ function patchHttp (http, tracer) {
}
}

function unpatch (http) {
shimmer.unwrap(http, 'request')

if (semver.satisfies(process.version, '>=8.0.0')) {
shimmer.unwrap(http, 'get')
}
}

module.exports = {
module: 'http',
OPERATION_NAME,
patch: patchHttp,
unpatch: unpatchHttp
patch,
unpatch
}
27 changes: 10 additions & 17 deletions src/instrumentation/httpClient.spec.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict'

const http = require('http')
const https = require('https')
const request = require('request-promise-native')
const nock = require('nock')
const { expect } = require('chai')
Expand All @@ -24,15 +23,10 @@ describe('instrumentation: httpClient', () => {
this.sandbox.stub(cls, 'startChildSpan').callsFake(() => mockChildSpan)

instrumentation.patch(http, tracer)
instrumentation.patch(https, tracer)

nock.disableNetConnect()
})

afterEach(() => {
instrumentation.unpatch(http)
instrumentation.unpatch(https)
nock.enableNetConnect()
nock.cleanAll()
})

Expand All @@ -53,39 +47,38 @@ describe('instrumentation: httpClient', () => {
})
})

it('should start and finish span with https', () => {
nock('https://risingstack.com')
.get('/')
.reply(200)
it('should start and finish span with https', () =>
// WARNING: nock doesn't work well with https instrumentation
// create real request

return request('https://risingstack.com')
request('https://risingstack.com')
.then(() => {
expect(cls.startChildSpan).to.be.calledWith(tracer, instrumentation.OPERATION_NAME)
expect(mockChildSpan.setTag).to.have.calledWith(Tags.HTTP_URL, 'http://risingstack.com:443')
expect(mockChildSpan.setTag).to.have.calledWith(Tags.HTTP_URL, 'https://risingstack.com:443')
expect(mockChildSpan.setTag).to.have.calledWith(Tags.HTTP_METHOD, 'GET')
expect(mockChildSpan.setTag).to.have.calledWith(Tags.SPAN_KIND_RPC_CLIENT, true)
expect(mockChildSpan.setTag).to.have.calledWith(Tags.HTTP_STATUS_CODE, 200)
expect(mockChildSpan.finish).to.have.callCount(1)
})
})
)

it('should flag wrong status codes as error', () => {
nock('https://risingstack.com')
nock('http://risingstack.com')
.get('/')
.reply(400)

return request('https://risingstack.com')
return request('http://risingstack.com')
.catch(() => {
expect(mockChildSpan.setTag).to.be.calledWith(Tags.ERROR, true)
})
})

it('should flag error', () => {
nock('https://risingstack.com')
nock('http://risingstack.com')
.get('/')
.replyWithError('My Error')

return request('https://risingstack.com')
return request('http://risingstack.com')
.catch((err) => {
expect(mockChildSpan.setTag).to.be.calledWith(Tags.ERROR, true)
expect(mockChildSpan.log).to.be.calledWith({
Expand Down
10 changes: 0 additions & 10 deletions src/instrumentation/httpsClient.js

This file was deleted.

35 changes: 0 additions & 35 deletions src/instrumentation/httpsClient.spec.js

This file was deleted.

2 changes: 0 additions & 2 deletions src/instrumentation/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@
const express = require('./express')
const expressError = require('./expressError')
const httpClient = require('./httpClient')
const httpsClient = require('./httpsClient')
const mongodbCore = require('./mongodbCore')
const pg = require('./pg')

module.exports = [
express,
expressError,
httpClient,
httpsClient,
mongodbCore,
pg
]
1 change: 0 additions & 1 deletion src/tracer.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ describe('Tracer', () => {

expect(instrumentationExpress.patch).to.be.calledWith(express, tracer._tracer)
expect(instrumentationHTTPClient.patch).to.be.calledWith(http, tracer._tracer)
expect(instrumentationHTTPClient.patch).to.be.calledWith(https, tracer._tracer)
})

it('should not apply instrumentation for not supported version', function () {
Expand Down

0 comments on commit 79ef8fb

Please sign in to comment.