feat: Add HTTP breadcrumbs. #508
Conversation
| @@ -12,6 +12,15 @@ export default class ErrorCollector implements Collector { | |||
| }, | |||
| true, | |||
| ); | |||
| window.addEventListener( | |||
There was a problem hiding this comment.
I was not handling unhandled promise rejections. Treating it the same as an error when possible.
…arkly/js-server-sdk-private into rlamb/sc-249563/http-breadcrumbs
kinyoklion
changed the title
| @@ -0,0 +1,84 @@ | |||
| import toSelector, { elementToString, getClassName } from '../../../src/collectors/dom/toSelector'; | |||
There was a problem hiding this comment.
Moved this file, but it should not have changes.
| * @param callback Function which handles a breadcrumb. | ||
| */ | ||
| export default function decorateFetch(callback: (breadcrumb: HttpBreadcrumb) => void) { | ||
| // TODO: Check if already wrapped? |
There was a problem hiding this comment.
There are a number of TODO items in this PR that will remain.
kinyoklion
requested review from
tanderson-ld and
keelerm84
and removed request for
a team
| * Customize URL filtering. This will be applied in addition to some baseline filtering included | ||
| * which redacts components of LaunchDarkly URLs. | ||
| */ | ||
| customUrlFilter?: UrlFilter; |
There was a problem hiding this comment.
We include a base filter, and then a customer an supply this additional filter. We may need more advanced filtering. For instance excluding entire requests, for a production version.
| try { | ||
| // Use defineProperties to prevent these values from being enumerable. | ||
| // The properties default to non-enumerable. | ||
| Object.defineProperties(window.XMLHttpRequest, { |
There was a problem hiding this comment.
Not doing anything with these yet, but will probably check for their presence in a future version. For cases where people instantiate multiple SDKs and telemetry.
packages/telemetry/browser-telemetry/src/filters/defaultUrlFilter.ts
Outdated
Show resolved
Hide resolved
| const streamingREgex = /\/eval\/[^/]*\/(?<context>[a-zA-Z0-9=]*)\??.*?/; | ||
|
|
||
| /** | ||
| * Filter which removes context information for browser JavaScript endpoints. |
There was a problem hiding this comment.
This is a sufficient implementation for a prototype. It isn't format agnostic, which means it could break easily.
We may want to filter to something more generic for production. For instance if the header includes one of the LD specified headers, then replace the URL with something like <LaunchDarkly-Polling> or other generic identifiers.
There was a problem hiding this comment.
Could just lopping off everything after the domain be sufficient for our customers? Then the filter could look for our domains, find end, truncate.
There was a problem hiding this comment.
Discussed more in another channel.
| @@ -85,7 +110,7 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry { | |||
| this.pendingEvents.shift(); | |||
| } | |||
| } | |||
| this.client?.track(`$ld:telemetry:${type}`, event); | |||
There was a problem hiding this comment.
Bug from earlier refactoring.
| } | ||
|
|
||
| unregister(): void { | ||
| this.destination = undefined; |
There was a problem hiding this comment.
this.destination being undefined after unregister is different than its original state before register was called. I know private destination: BrowserTelemetry | undefined is possible, but I'm unsure of the pros and cons. Perhaps there is no appreciable difference.
There was a problem hiding this comment.
Now I see elsewhere there is init?: RequestInit | undefined
There was a problem hiding this comment.
A field before registration is undefined and after unregistration is undefined.
private destination?: BrowserTelemetry;
is the same as
private destination: BrowserTelemetry | undefined
As the field is not defined in the constructor it will be undefined, so it is conditionally accessed.
It isn't assigned in the constructor because I want to be able to make collectors immediately and then attach them.
The second thing is just because it was defined that way in the lib.dom.ts and I copied the definition.
There was a problem hiding this comment.
Generally speaking optional elements cannot follow non-optional elements, so there is another case where it is a parameter is non-optional, but can be undefined. Which just means you have to pass something.
| * which is completed. | ||
| */ | ||
| export default class XhrCollector implements Collector { | ||
| private destination?: BrowserTelemetry; |
There was a problem hiding this comment.
Same question here wrt to undefined-ness
| const streamingREgex = /\/eval\/[^/]*\/(?<context>[a-zA-Z0-9=]*)\??.*?/; | ||
|
|
||
| /** | ||
| * Filter which removes context information for browser JavaScript endpoints. |
There was a problem hiding this comment.
Could just lopping off everything after the domain be sufficient for our customers? Then the filter could look for our domains, find end, truncate.
| const streamingREgex = /\/eval\/[^/]*\/(?<context>[a-zA-Z0-9=]*)\??.*?/; | ||
|
|
||
| /** | ||
| * Filter which removes context information for browser JavaScript endpoints. |
There was a problem hiding this comment.
Discussed more in another channel.
Prototype implementation.