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.

Sign up for GitHub

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

Jump to bottom

Remove requestContentLength metadata from network breadcrumbs #1983

Closed
wants to merge 1 commit into from
Closed

Remove requestContentLength metadata from network breadcrumbs #1983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 13 additions & 56 deletions packages/plugin-network-breadcrumbs/network-breadcrumbs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ module.exports = (_ignoredUrls = [], win = window) => {
win.XMLHttpRequest.prototype.open = function open (method, url) {
let requestSetupKey = false

const error = () => handleXHRError(method, url, getDuration(requestStart), requestContentLength)
const load = () => handleXHRLoad(method, url, this.status, getDuration(requestStart), requestContentLength)
const error = () => handleXHRError(method, url, getDuration(requestStart))
const load = () => handleXHRLoad(method, url, this.status, getDuration(requestStart))

// if we have already setup listeners, it means open() was called twice, we need to remove
// the listeners and recreate them
Expand All @@ -47,11 +47,9 @@ module.exports = (_ignoredUrls = [], win = window) => {

const oldSend = this.send
let requestStart
let requestContentLength

// override send() for this XMLHttpRequest instance
this.send = function send (body) {
requestContentLength = getByteLength(body)
this.send = function send () {
requestStart = new Date()
oldSend.apply(this, arguments)
}
Expand All @@ -66,7 +64,7 @@ module.exports = (_ignoredUrls = [], win = window) => {
}
}

function handleXHRLoad (method, url, status, duration, requestContentLength) {
function handleXHRLoad (method, url, status, duration) {
if (url === undefined) {
client._logger.warn('The request URL is no longer present on this XMLHttpRequest. A breadcrumb cannot be left for this request.')
return
Expand All @@ -81,8 +79,7 @@ module.exports = (_ignoredUrls = [], win = window) => {
const metadata = {
status: status,
request: `${method} ${url}`,
duration: duration,
requestContentLength: requestContentLength
duration: duration
}
if (status >= 400) {
// contacted server but got an error response
Expand All @@ -92,7 +89,7 @@ module.exports = (_ignoredUrls = [], win = window) => {
}
}

function handleXHRError (method, url, duration, requestContentLength) {
function handleXHRError (method, url, duration) {
if (url === undefined) {
client._logger.warn('The request URL is no longer present on this XMLHttpRequest. A breadcrumb cannot be left for this request.')
return
Expand All @@ -106,8 +103,7 @@ module.exports = (_ignoredUrls = [], win = window) => {
// failed to contact server
client.leaveBreadcrumb('XMLHttpRequest error', {
request: `${method} ${url}`,
duration: duration,
requestContentLength: requestContentLength
duration: duration
}, BREADCRUMB_TYPE)
}

Expand All @@ -125,7 +121,6 @@ module.exports = (_ignoredUrls = [], win = window) => {

let method
let url = null
let requestContentLength

if (urlOrRequest && typeof urlOrRequest === 'object') {
url = urlOrRequest.url
Expand All @@ -145,21 +140,17 @@ module.exports = (_ignoredUrls = [], win = window) => {
method = 'GET'
}

if (options && 'body' in options) {
requestContentLength = getByteLength(options.body)
}

return new Promise((resolve, reject) => {
const requestStart = new Date()

// pass through to native fetch
oldFetch(...arguments)
.then(response => {
handleFetchSuccess(response, method, url, getDuration(requestStart), requestContentLength)
handleFetchSuccess(response, method, url, getDuration(requestStart))
resolve(response)
})
.catch(error => {
handleFetchError(method, url, getDuration(requestStart), requestContentLength)
handleFetchError(method, url, getDuration(requestStart))
reject(error)
})
})
Expand All @@ -172,12 +163,11 @@ module.exports = (_ignoredUrls = [], win = window) => {
}
}

const handleFetchSuccess = (response, method, url, duration, requestContentLength) => {
const handleFetchSuccess = (response, method, url, duration) => {
const metadata = {
status: response.status,
request: `${method} ${url}`,
duration: duration,
requestContentLength: requestContentLength
duration: duration
}
if (response.status >= 400) {
// when the request comes back with a 4xx or 5xx status it does not reject the fetch promise,
Expand All @@ -187,12 +177,8 @@ module.exports = (_ignoredUrls = [], win = window) => {
}
}

const handleFetchError = (method, url, duration, requestContentLength) => {
client.leaveBreadcrumb('fetch() error', {
request: `${method} ${url}`,
duration: duration,
requestContentLength: requestContentLength
}, BREADCRUMB_TYPE)
const handleFetchError = (method, url, duration) => {
client.leaveBreadcrumb('fetch() error', { request: `${method} ${url}`, duration: duration }, BREADCRUMB_TYPE)
}
}
}
Expand All @@ -204,35 +190,6 @@ module.exports = (_ignoredUrls = [], win = window) => {
}
}

const getByteLength = (body) => {
// body could be any of the types listed here: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send#parameters
// ReadableStreams cannot be read safely and it's difficult to get an accurate byte length for FormData and Document inputs
if ((body === null || typeof body === 'undefined') ||
(win.ReadableStream && body instanceof win.ReadableStream) ||
(win.FormData && body instanceof win.FormData) ||
(win.Document && body instanceof win.Document)) return undefined

// Try to read the byte length directly
if (typeof body.byteLength === 'number') {
// ArrayBuffer, DataView, TypedArray
return body.byteLength
} else if (win.Blob && body instanceof win.Blob) {
return body.size
} else if (!win.Blob) {
return undefined
}

try {
// do a simple stringification - this may fail if the input object has no prototype or a broken toString
const stringified = String(body)

// use a Blob to get the utf-8 encoded byte length
return new win.Blob([stringified]).size
} catch (e) {
return undefined
}
}

return plugin
}

Expand Down
43 changes: 15 additions & 28 deletions packages/plugin-network-breadcrumbs/test/network-breadcrumbs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class XMLHttpRequest {
open (method: string, url: string | { toString: () => any }) {
}

send (body: any, fail: boolean = false, status: number | null = null) {
send (fail: boolean, status: number | null = null) {
if (fail) {
this._listeners.error.call(this)
} else {
Expand Down Expand Up @@ -73,7 +73,7 @@ describe('plugin: network breadcrumbs', () => {
const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
request.open('GET', '/')
// tell the mock request to succeed with status code 200
request.send(undefined, false, 200)
request.send(false, 200)

expect(client._breadcrumbs.length).toBe(1)
expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({
Expand All @@ -85,7 +85,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
})

it('should not leave duplicate breadcrumbs if open() is called twice', () => {
Expand All @@ -97,7 +96,7 @@ describe('plugin: network breadcrumbs', () => {
const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
request.open('GET', '/')
request.open('GET', '/')
request.send(undefined, false, 200)
request.send(false, 200)
expect(client._breadcrumbs.length).toBe(1)
})

Expand All @@ -109,7 +108,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
request.open('GET', '/this-does-not-exist')
request.send(undefined, false, 404)
request.send(false, 404)

expect(client._breadcrumbs.length).toBe(1)
expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({
Expand All @@ -121,7 +120,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
})

it('should leave a breadcrumb when an XMLHTTPRequest has a network error', () => {
Expand All @@ -133,7 +131,7 @@ describe('plugin: network breadcrumbs', () => {
const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest

request.open('GET', 'https://another-domain.xyz/')
request.send(undefined, true)
request.send(true)

expect(client._breadcrumbs.length).toBe(1)
expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({
Expand All @@ -144,7 +142,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
})

it('should not leave a breadcrumb for request to bugsnag notify endpoint', () => {
Expand All @@ -155,7 +152,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
request.open('GET', client._config.endpoints!.notify)
request.send(undefined, false, 200)
request.send(false, 200)

expect(client._breadcrumbs.length).toBe(0)
})
Expand All @@ -168,7 +165,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
request.open('GET', client._config.endpoints!.sessions)
request.send(undefined, false, 200)
request.send(false, 200)
expect(client._breadcrumbs.length).toBe(0)
})

Expand All @@ -187,7 +184,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
request.open('GET', { toString: () => 'https://example.com' })
request.send(undefined, false, 200)
request.send(false, 200)

expect(client._breadcrumbs.length).toBe(1)
expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({
Expand All @@ -199,7 +196,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
})

it('should leave a breadcrumb when the request URL is not a string for a request that errors', () => {
Expand All @@ -217,7 +213,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new window.XMLHttpRequest() as unknown as XMLHttpRequest
request.open('GET', { toString: () => 'https://example.com' })
request.send(undefined, true)
request.send(true)

expect(client._breadcrumbs.length).toBe(1)
expect(client._breadcrumbs[0]).toEqual(expect.objectContaining({
Expand Down Expand Up @@ -248,7 +244,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -271,7 +266,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -296,7 +290,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -321,7 +314,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -344,7 +336,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand Down Expand Up @@ -433,7 +424,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -456,7 +446,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -479,7 +468,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -501,7 +489,6 @@ describe('plugin: network breadcrumbs', () => {
duration: expect.any(Number)
}
}))
expect(client._breadcrumbs[0].metadata.requestContentLength).toBeUndefined()
done()
})
})
Expand All @@ -514,7 +501,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new XMLHttpRequest()
request.open('GET', '/')
request.send(undefined, false, 200)
request.send(false, 200)

expect(client._breadcrumbs.length).toBe(0)
})
Expand All @@ -527,7 +514,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new XMLHttpRequest()
request.open('GET', '/')
request.send(undefined, false, 200)
request.send(false, 200)

expect(client._breadcrumbs.length).toBe(1)
})
Expand All @@ -540,7 +527,7 @@ describe('plugin: network breadcrumbs', () => {

const request = new XMLHttpRequest()
request.open('GET', '/')
request.send(undefined, false, 200)
request.send(false, 200)

expect(client._breadcrumbs.length).toBe(1)
})
Expand All @@ -553,15 +540,15 @@ describe('plugin: network breadcrumbs', () => {

const request0 = new XMLHttpRequest()
request0.open('GET', '/')
request0.send(undefined, false, 200)
request0.send(false, 200)

const request1 = new XMLHttpRequest()
request1.open('GET', '/ignoreme?123')
request1.send(undefined, false, 200)
request1.send(false, 200)

const request2 = new XMLHttpRequest()
request2.open('GET', '/ignoremeno')
request2.send(undefined, false, 200)
request2.send(false, 200)

expect(client._breadcrumbs.length).toBe(2)
})
Expand Down
2 changes: 1 addition & 1 deletion test/browser/Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
source 'https://rubygems.org'

gem 'bugsnag-maze-runner', git: 'https://github.com/bugsnag/maze-runner', tag: 'v7.17.0'
gem 'bugsnag-maze-runner', git: 'https://github.com/bugsnag/maze-runner', tag: 'v7.8.0'

# Use a branch of Maze Runner
#gem 'bugsnag-maze-runner', git: 'https://github.com/bugsnag/maze-runner', branch: 'tms/use-maze-check'
Expand Down
Loading
Loading