Skip to content

Commit

Permalink
Revert "add requestContentLength breadcrumb metadata for xhr and fetc…
Browse files Browse the repository at this point in the history 
…h requests"
  • Loading branch information
@yousif-bugsnag
yousif-bugsnag authored Jul 6, 2023
1 parent 630d9ca commit b1904cb
Show file tree
Hide file tree
Showing 35 changed files with 56 additions and 752 deletions.
69 changes: 13 additions & 56 deletions packages/plugin-network-breadcrumbs/network-breadcrumbs.js
View file
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
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
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

0 comments on commit b1904cb

Please sign in to comment.