✨ [RUM-8780] Make developer extension capable of injecting the browser SDK #3671
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3671 +/- ##
==========================================
- Coverage 92.19% 92.15% -0.04%
==========================================
Files 331 331
Lines 8286 8288 +2
Branches 1867 1868 +1
==========================================
- Hits 7639 7638 -1
- Misses 647 650 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
This reverts commit 55fddcc.
BeltranBulbarellaDD
force-pushed
the
beltran.bulbarella/RUM-8780
branch
from
8a4b6c3 to
2d75031
Compare
| - **Request Interception**: override the current SDK bundle with local build, or switch between `rum` and `rum-slim` bundles on any site that is using RUM SDK. (note: if the SDK is installed from NPM, this override might not work, as it is still in an experimental stage.) | ||
|
|
||
| - **Trial Mode**: Access SDK injection capabilities through the Trial tab. When enabled, you can configure and inject the Datadog Browser SDK into pages that don't have it. Configure your Application ID, Client Token, site, and other parameters to test SDK features on any website. |
There was a problem hiding this comment.
No action requested in this PR, but: "Request Interception" and "Trial Mode" feel like they have a bit of overlap, since they both involve injection. I don't think it's necessary to change things in this PR, because handling it cleanly would probably require changes to the some of the wording in the extension UI, but it'd be nice to rename things a bit to clarify when you would use each feature. It feels like "Request Interception" is really "SDK Version Swapping", or something like that.
Edit: actually, looking at the settings panel changes, I'm starting to think that maybe we should address this issue in this PR. We may want to outright remove existing features that are duplicated by Trial Mode, so that there's a single clear place to accomplish each task.
| chrome.storage.local.get(null, (allSettings) => { | ||
| sendResponse({ settings: allSettings }) | ||
| }) | ||
| return true |
There was a problem hiding this comment.
Nit: IMO it'd be nicer to return a Promise, which AFAIK is also supported. I had to look up what return true mean to understand what was happening here.
There was a problem hiding this comment.
Promise returned in function argument where a void return was expected @typescript-eslint/no-misused-promises
So I am having issues returning a Promise. I could eslint-ignore it.
|
|
||
| export interface SdkInjectionConfig { | ||
| enabled: boolean | ||
| sdkTypes: Array<'rum' | 'logs'> |
There was a problem hiding this comment.
Nit: maybe Record<'rum' | 'logs', boolean> would be a bit nicer.
| `if (!scriptUrl) scriptUrl = ${JSON.stringify(url)};` | ||
| ) | ||
| // Fallback – if the above pattern did not match (future wording change), explicitly set the | ||
| // webpack public path at the very top of the bundle so chunk loading keeps working. |
There was a problem hiding this comment.
If this is reliable, heck, we could just get rid of the old method!
| <> | ||
| <Space h="lg" /> | ||
| <Text size="lg" fw={600}> | ||
| SDK Injection Configuration |
There was a problem hiding this comment.
It feels a bit confusing that we're using the term "injection" both here and as one of the override strategies on the other page, and that one prevents you from using the other, even though they're very nearly the same thing. I think this ties back to the concern I had that the two features are confusingly similar in the docs, but I think the issue is even more visible when looking at how the different settings panels interact.
Perhaps we should just get rid of the override stuff that's duplicated by Trial Mode?
| } | ||
| // intake request path is /api/vX/track | ||
| if (!['rum', 'logs'].includes(url.pathname.split('/')[3])) { | ||
| // Accept various intake path formats: older /api/vX/track, newer /v1/input, or anything containing /rum or /logs. |
There was a problem hiding this comment.
It seems like you are low-key fixing a bunch of existing bugs here, in addition to implementing Trial Mode. 😄
| function setSetting<Name extends keyof Settings>(name: Name, value: Settings[Name]) { | ||
| settings![name] = value | ||
|
|
||
| // Special case: keep bundleSource and sdkInjection.bundleSource in sync |
There was a problem hiding this comment.
Another indication that we might want to consolidate things.
BeltranBulbarellaDD
changed the title
BeltranBulbarellaDD
changed the title
BeltranBulbarellaDD
changed the title
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 364a2ff | Docs | Was this helpful? Give us feedback! |
Motivation
The idea is to be able to inject the SDK on a page that does not have it. Injecting the local version and CDN version.
Changes
Added a Trial tab that allows to configure the SDK to be injected.
If CDN injection is enabled, the setting's tab gets set to:
Event collection strategy: RequestsandAuto Flushas it is needed for CDN.I can note an issue which is that when I
Disable event intake (do not send data to Datadog)for which I do:{ beforeSend: () => false, }For which the initial view gets created but no other information gets sent.
Test instructions
Injecting using local version:
https://github.com/user-attachments/assets/b63b6fc2-5901-4703-88d3-798d9ea4be49
Injecting CDN version:
https://github.com/user-attachments/assets/874df7c6-b921-40eb-b01d-84fbba7a2423
Testing Session Replay tab on CDN injection.
https://github.com/user-attachments/assets/edab2e73-f596-428f-b89b-29a8f6d21d23
Checklist