[plugin] RUM plugin #78

yoannmoinet · 2024-06-06T12:32:54Z

Note

Stacked on #82

What and why?

Adding a new customer facing plugin to let them upload sourcemaps at the end of a build.

rum?: {
    disabled?: boolean;
    sourcemaps?: {
        basePath: string;
        dryRun?: boolean;
        maxConcurrency?: number;
        minifiedPathPrefix?: string;
        releaseVersion: string;
        service: string;
    };
}

I followed datadog-ci's implementation but re-wrote it entirely to make it leverage native primitives (like fetch). And straightened its helpers.

I could have imported datadog-ci directly, but this was bringing A TON of unnecessary dependencies, so I decided to re-wrote it.

Note

This also slightly changes how the git internal plugin works. It will only enable it if the sourcemaps plugin is enabled.

How?

This is a sub-plugin of the higher order rum plugin.
Added some configuration validation, as well as a lot of tests for the added functionality.

I've also split out the git internal plugin in #82 so we keep this one more digest to review.

The implementation is pretty straight-forward:

glob sourcemaps files.
construct payloads.
send it all to Datadog.

In the future I'd like to:

deduct the basePath from the bundler's config.
make it possible to upload to more than one DC in one go.
remove the dependency on form-data to use the native one instead.

Finally, I tested the upload and it seemed to work as expected.

packages/plugins/rum/src/sourcemaps/files.ts

yoannmoinet · 2024-06-24T09:28:29Z

packages/plugins/rum/src/sourcemaps/sender.ts

+        // Using form-data as a dependency isn't really required.
+        // But this implementation makes it mandatory for the gzip step.
+        // NodeJS' fetch SHOULD support everything else from the native FormData primitive.
+        // content.options are totally optional and should only be filename.
+        // form.getHeaders() is natively handled by fetch and FormData.


This one is a toughy, so any help will be appreciated.
Not blocking, but would be great to remove this dependency.

Miz85

LGTM! I have a few comments/suggestions but otherwise it looks good

Miz85 · 2024-06-24T09:44:32Z

.github/CODEOWNERS

@@ -6,3 +6,7 @@
 # Telemetry
 packages/plugins/telemetry                                            @DataDog/frontend-devx @yoannmoinet
 packages/tests/src/plugins/telemetry                                  @DataDog/frontend-devx @yoannmoinet
+
+# Rum
+packages/plugins/rum                                                  @DataDog/rum @yoannmoinet


Should probably fix these.

Do you think we have a good team to add for RUM?
This one seemed a bit too wide maybe.

Miz85 · 2024-06-24T09:49:21Z

packages/plugins/rum/src/index.ts

+export const helpers = {
+    // Add the helpers you'd like to expose here.
+};


Could we remove this? It seems like it's not used

Haha, I was just thinking of this, I think right now we can't remove it, as the publish script expect all the plugins to have this.
But I'll submit a separate PR to make this optional, and only use the ones that are existing.

Miz85 · 2024-06-24T09:50:47Z

packages/plugins/rum/src/sourcemaps/files.ts

+    // Always using the posix version to avoid \ on Windows.
+    const newPath = path.posix.normalize(basePath);
+    return path.join(newPath, '**/*.@(js|mjs).map');


question: Should the pattern be configurable?

I followed datadog-ci's implementation, where this is not configurable.

https://github.com/DataDog/datadog-ci/blob/026557808bb4ca0756bac078b715f63c9c11bca7/src/commands/sourcemaps/upload.ts#L173-L174

I prefer to cross that bridge when we get there for simplicity's sake.

Miz85 · 2024-06-24T09:55:47Z

packages/plugins/rum/src/sourcemaps/sender.ts

+
+export const getApiUrl = (context: GlobalContext) => {
+    const siteUrl = context.auth?.endPoint || 'datadoghq.com';
+    return `https://sourcemap-intake.${siteUrl}/api/v2/srcmap`;


suggestion: We could also make this pattern configurable if there's any chance it could change.

Same as prior, followed datadog-ci's implementation, where it's not configurable (yet).

https://github.com/DataDog/datadog-ci/blob/026557808bb4ca0756bac078b715f63c9c11bca7/src/helpers/base-intake-url.ts#L1-L9

I do see where it could be valuable to make it configurable, but I prefer to cross this bridge when we get there.

For the sake of simplicity.

ps: I'll add support for env vars in a separate PR as well.

🤔 this may be actually needed before we release this plugin after all, given that datadog-ci's offers the DATADOG_SOURCEMAP_INTAKE_URL.

Added it in the last commit 19e3862

[testing] Refactor testing telemetry

[core] More global contexts

[rum] Use context for sourcemaps instead of `basePath`.

yoannmoinet force-pushed the yoann/rum-plugin branch from de6d30d to 64d26cf Compare June 7, 2024 14:27

yoannmoinet changed the base branch from master to yoann/add-plugin-and-test-helpers June 7, 2024 14:27

Base automatically changed from yoann/add-plugin-and-test-helpers to master June 10, 2024 09:56

yoannmoinet force-pushed the yoann/rum-plugin branch 2 times, most recently from b477374 to 795c522 Compare June 10, 2024 15:01

yoannmoinet changed the base branch from master to yoann/move-telemetry June 10, 2024 15:01

Base automatically changed from yoann/move-telemetry to master June 10, 2024 15:32

yoannmoinet force-pushed the yoann/rum-plugin branch from 3951241 to d581174 Compare June 20, 2024 14:16

yoannmoinet changed the base branch from master to yoann/core-plugins-update June 20, 2024 14:16

DataDog deleted a comment from datadog-datadog-staging-us1-all bot Jun 20, 2024

yoannmoinet force-pushed the yoann/core-plugins-update branch from 5bfc8c0 to c5e6849 Compare June 20, 2024 16:04

yoannmoinet added 3 commits June 24, 2024 10:58

Add sourcemaps plugin

30871ce

Automation

fb56bc8

Update when we inject git data

e824eef

yoannmoinet force-pushed the yoann/rum-plugin branch from d581174 to e824eef Compare June 24, 2024 08:58

yoannmoinet mentioned this pull request Jun 24, 2024

[core] core plugins update #82

Merged

yoannmoinet commented Jun 24, 2024

View reviewed changes

packages/plugins/rum/src/sourcemaps/files.ts Outdated Show resolved Hide resolved

yoannmoinet commented Jun 24, 2024

View reviewed changes

DataDog deleted a comment from datadog-datadog-staging-us1-all bot Jun 24, 2024

Update packages/plugins/rum/src/sourcemaps/files.ts

263640f

yoannmoinet marked this pull request as ready for review June 24, 2024 09:32

DataDog deleted a comment from datadog-datadog-staging-us1-all bot Jun 24, 2024

Miz85 reviewed Jun 24, 2024

View reviewed changes

Add intake override and test it

19e3862

Base automatically changed from yoann/core-plugins-update to master June 25, 2024 07:58

yoannmoinet added 3 commits June 25, 2024 10:12

Merge remote-tracking branch 'origin/master' into yoann/rum-plugin

38bc7d7

Run integrity

e69c1b8

Update endPoint usage

8d90ef3

elbywan approved these changes Jun 25, 2024

View reviewed changes

Update how we compute the body

9271c7c

yoannmoinet added 5 commits July 9, 2024 16:55

Add outputFiles to globalContext

0ca8c30

Fix telemetry tests

14d5160

Add webpack4 support to outputFiles context

94439e3

Run tests with webpack4 as well

e7e87c3

Fix rollupConfig.mjs import

7bc1843

yoannmoinet mentioned this pull request Jul 10, 2024

[testing] Refactor testing telemetry #85

Merged

yoannmoinet and others added 22 commits July 10, 2024 09:59

Add outputDir to context for webpack and esbuild

5b2192e

Update test

b8a4b48

Fix outputFiles global context as it was not filled in time for webpack

5fc71c0

Make jest silent

8fe118c

Run all bundlers

22db40c

Use context instead of basePath for sourcemaps

35aa4a6

Clean builds before running bundlers

54ab14b

Update tests

b61d5b5

Fix lock

48bf921

Remove comment

92f7158

Update scripts for loops

b99b3c9

Change error log

f86c49e

Update test with context to actually reflect context's mutable object

0a37000

Add test:noisy

69bcdc5

Webpack 4 is hanging, delay the resolution

08b9010

Remove .only

8865781

Merge pull request #85 from DataDog/yoann/refactor-testing-telemetry

5952bad

[testing] Refactor testing telemetry

Merge pull request #84 from DataDog/yoann/more-global-contexts

b2ca98d

[core] More global contexts

Update output

a92830f

Merge pull request #87 from DataDog/yoann/use-context-for-sourcemaps

d6aaea2

[rum] Use context for sourcemaps instead of `basePath`.

Update unplugin to latest

872ab89

v2.1.0

500f410

yoannmoinet merged commit accae07 into master Jul 10, 2024
5 checks passed

yoannmoinet deleted the yoann/rum-plugin branch July 10, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[plugin] RUM plugin #78

[plugin] RUM plugin #78

yoannmoinet commented Jun 6, 2024 •

edited

Loading

yoannmoinet Jun 24, 2024 •

edited

Loading

Miz85 left a comment

Miz85 Jun 24, 2024

yoannmoinet Jun 24, 2024

Miz85 Jun 24, 2024

yoannmoinet Jun 24, 2024

Miz85 Jun 24, 2024

yoannmoinet Jun 24, 2024 •

edited

Loading

Miz85 Jun 24, 2024

yoannmoinet Jun 24, 2024

yoannmoinet Jun 24, 2024

yoannmoinet Jun 24, 2024

yoannmoinet Jun 24, 2024

[plugin] RUM plugin #78

[plugin] RUM plugin #78

Conversation

yoannmoinet commented Jun 6, 2024 • edited Loading

What and why?

How?

yoannmoinet Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Miz85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoannmoinet Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoannmoinet commented Jun 6, 2024 •

edited

Loading

yoannmoinet Jun 24, 2024 •

edited

Loading

yoannmoinet Jun 24, 2024 •

edited

Loading