Inject the telemetry token into the Devtools browser extension during build

[chentong7]

Description

In order for the Devtools browser extension to successfully send usage telemetry to Aria/Kusto, it needs a token. We don't want to put that token in source control if we can avoid it, to minimize the risk of someone obtaining it and abusing it. We should find a way to inject this value into the extension code during build (both local and CI).
AB#5167

This change will also upload the injected key and built Devtools browser extension into build client pipeline artifact.

Example

[tylerbutler]

This PR sets the project up to read the env variables from a file, but where is the value set in the CI pipeline?

[alexvy86]

A few comments.

Also, this would solve the injection during local build but I just realized there's nothing in our pipelines that would produce the npm-package/bundle for this package because of private: true in package.json (which we should keep), so with this change as is, we still need to produce the bundles for the browser extension stores locally.

I see two options to address that:

• Add a few new steps to the existing Build - client packages pipeline: one to build only this package, providing the necessary env variable for the token, and a second one to publish a pipeline artifact with the generated bundle, that we could then directly publish to the browser extension stores.
• Create a new pipeline with the same two steps above.

I lean towards the first one, which has the advantage that the extra steps wouldn't have to rebuild a good chunk of the repo. @Josmithr thoughts?

[Josmithr]

Let's be sure to add clear instructions to the package README(s) about the required environment variables and how to set them up when testing locally (including when uploading a local extension bundle to the browser, not just when running the example app).

[chentong7]

This PR sets the project up to read the env variables from a file, but where is the value set in the CI pipeline?

It's set in https://dev.azure.com/fluidframework/internal/_library?itemType=VariableGroups&view=VariableGroupView&variableGroupId=5&path=prague-key-vault. Will be download in pipeline step Download secrets: prague-key-vault(https://dev.azure.com/fluidframework/internal/_build/results?buildId=190325&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=e77055a3-6358-5204-c080-7a2e41553284)

[alexvy86]

This PR sets the project up to read the env variables from a file, but where is the value set in the CI pipeline?

It's set in https://dev.azure.com/fluidframework/internal/_library?itemType=VariableGroups&view=VariableGroupView&variableGroupId=5&path=prague-key-vault. Will be download in pipeline step Download secrets: prague-key-vault(https://dev.azure.com/fluidframework/internal/_build/results?buildId=190325&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=e77055a3-6358-5204-c080-7a2e41553284)

I think what Tyler means is that this PR does not have anything that will actually use the value that we made available for ADO pipeline runs, when building/bundling the devtools extension. We added a use for it in the step that calculates the bundle size (which I'm still not convinced is necessary), but we need to make it available when building this package. My previous comment touches on this topic.

[chentong7]

A few comments.

Also, this would solve the injection during local build but I just realized there's nothing in our pipelines that would produce the npm-package/bundle for this package because of private: true in package.json (which we should keep), so with this change as is, we still need to produce the bundles for the browser extension stores locally.

I see two options to address that:

• Add a few new steps to the existing Build - client packages pipeline: one to build only this package, providing the necessary env variable for the token, and a second one to publish a pipeline artifact with the generated bundle, that we could then directly publish to the browser extension stores.
• Create a new pipeline with the same two steps above.

I lean towards the first one, which has the advantage that the extra steps wouldn't have to rebuild a good chunk of the repo. @Josmithr thoughts?

I added a pipeline step in existing Build - client packages to prepare the .env config file in build machine agent then the build step will replace DEVTOOLS_TELEMETRY_TOKEN with the token value we set in prague-key-vault. First option works.

[chentong7]

Let's be sure to add clear instructions to the package README(s) about the required environment variables and how to set them up when testing locally (including when uploading a local extension bundle to the browser, not just when running the example app).

Updated the document about it

[alexvy86]

For local development, I think we don't want a fake token injected automatically because that will still enable telemetry (line 100 in TelemetryLogging.ts) and cause requests to be issued to the collection endpoint. The requests won't go through because the token is invalid, but ideally we shouldn't be making them. So I'd say:

• Let's remove the dev-example.env altogether.
• In webpack.test.js, change the path we pass to Dotenv so it uses ./.env just like the production one.
• In the README, the instructions to test telemetry locally should say "create an .env file with contents that look like . Then npm run start:client:test will inject the token from that .env file into the extension bundle".
• Add an entry for .env to the package's .gitignore file, so any such file someone creates locally isn't tracked by git and isn't at risk of being committed/pushed.

[chentong7]

For local development, I think we don't want a fake token injected automatically because that will still enable telemetry (line 100 in TelemetryLogging.ts) and cause requests to be issued to the collection endpoint. The requests won't go through because the token is invalid, but ideally we shouldn't be making them. So I'd say:

• Let's remove the dev-example.env altogether.
• In webpack.test.js, change the path we pass to Dotenv so it uses ./.env just like the production one.
• In the README, the instructions to test telemetry locally should say "create an .env file with contents that look like . Then npm run start:client:test will inject the token from that .env file into the extension bundle".
• Add an entry for .env to the package's .gitignore file, so any such file someone creates locally isn't tracked by git and isn't at risk of being committed/pushed.

Updated accordingly. Thank you very much for your detailed instruction. Today I learned👍

[alexvy86]

A few adjustments but this is looking almost ready!

[alexvy86]

One last thing, but I think otherwise it's ready to go! Let's apply that change, run the pipeline, and once we confirm that it is publishing the artifact and that the injected key is there, this is good to merge.

[Josmithr]

Do devs need to manually create this file and add the key? Or does npm start create the file / contents automatically?

[alexvy86]

I think it should be always manual when done locally. Unless we need a valid token locally for some reason (testing, most probably), the .env file doesn't need to exist (I'd argue maybe even slightly better that it doesn't exist) so I wouldn't make the npm scripts create one.

[Josmithr]

Nit: I don't think you need to include the note about "designing and implementing" this behavior in the PR title. Something like "Inject the telemetry token into the Devtools browser extension during build" is probably enough.

It would also probably be best to include a note in the PR description that we are now also publishing the browser extension artifact as a part of the PR/CI pipeline.

[tylerbutler]

It feels like this process could be simplified. In particular:

Instead of running a separate webpack step for this package, set the DEVTOOLS_TELEMETRY_TOKEN environment variable for the webpack/build steps in the main build. As it is, the webpack step might do nothing because fluid-build might not know that the contents of the .env file matters when deciding whether to rebuild.

I assume that if the environment variable is set, then that value is used by the build. If it is not, then we should make the build use the env variable (I think @alexvy86 made a fix for this recently) and error if it is not set.

Using a .env file to make the local testing experience better makes sense, but in CI we should prefer env variables (lots of examples in the pipelines where we do this). If needed, the package can provide a separate ci:build script that is used in CI to change the build in some way (e.g. local build script sets the environment variables so that local builds don't fail, but the ci:build script assumes the value will be set in the CI env).

I won't block merging this as-is, but IMO it should be simplified soon. The approach of adding individual steps for a pipeline isn't bad per se, but it doesn't seem needed here.

If you decide to merge as is, I still encourage you to bake more of the logic into the package.json script(s) instead of the CI workflow. The less CI code/config you're writing, the better. For example, writing the env variable to a .env file could be a script in the package. Less CI-only code == better.

[alexvy86]

As it is, the webpack step might do nothing because fluid-build might not know that the contents of the .env file matters when deciding whether to rebuild.

I raised this concern if we were using npm run build, which indeed would do nothing. npm run webpack directly works around that because it bundles no matter the current state of the world (i.e. previous builds).

I deliberately suggested not adding the token to the main build steps because the template is also used for other pipelines that don't care about the client release group (e.g. the different tools, common/). It would work, but I don't like that the template ends up incorporating bits and pieces that are only relevant to some of its "callers".

I assume that if the environment variable is set, then that value is used by the build.

We still need to pass it to a given ADO task (the one doing the build) through env:, by name, with the value set from the ADO variable. Which comes back to my concern about the template having bits that are only relevant to one of its "callers".

Using a .env file to make the local testing experience better makes sense, but in CI we should prefer env variables

Agree on that. Once my fix in build-tools is consumed in client, I'd rather see a solution that passes it like that instead of an .env file.

the package can provide a separate ci:build script

I didn't think of this one but it does feel tempting... do we know if it works today? Looking at fluidBuild.config.cjs where that task is marked as script: false makes me think it itself wouldn't be run...?

[tylerbutler]

but I don't like that the template ends up incorporating bits and pieces that are only relevant to some of its "callers".

@alexvy86 This is already true. See the include-vars template - not every pipeline uses every variable defined there.

I didn't think of this one but it does feel tempting... do we know if it works today?

It should work, but it may require adding something to the local package's fluid-build settings to make it look something like this:

	"fluidBuild": {
		"tasks": {
			"compile": {
				"dependsOn": [
					"...",
					"webpack"
				],
				"script": false
			},
			"ci:build": {
				"dependsOn": [
					"compile",
					"eslint",
					"env-var"
				],
				"script": false
			}
		}
	},

where the env-var script is the one that reads and sets the env variable.