Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add app insights #23303

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add app insights #23303

wants to merge 18 commits into from

Conversation

WayneFerrao
Copy link
Contributor

@WayneFerrao WayneFerrao commented Dec 11, 2024

Description

This PR adds user telemetry to the website through the integration of AppInsights

Changes include:

  • Added AppInsights config
  • Updated footer text for privacy compliance.

@github-actions github-actions bot added area: website base: main PRs targeted against main branch labels Dec 11, 2024
@WayneFerrao WayneFerrao requested review from Abe27342 and a team December 11, 2024 22:31
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can do this in the repo, some of the new deps are not publicly available so it becomes impossible for external contributors to work on docs/. I don't see how we would do this as extra steps in internal pipelines, though 🤔 .

@WayneFerrao
Copy link
Contributor Author

I don't think we can do this in the repo, some of the new deps are not publicly available so it becomes impossible for external contributors to work on docs/. I don't see how we would do this as extra steps in internal pipelines, though 🤔 .

Shoot good point, the wcp-consent code would need to be abstracted. I think the App Insights code can stay though

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  171689 links
    1596 destination URLs
    1826 URLs ignored
       0 warnings
       0 errors


@@ -85,7 +85,9 @@ function FooterSocialLinks(): JSX.Element {
function FooterPrivacyLinks(): JSX.Element {
return (
<div className="ffcom-footer-privacy">
<LinkItem targetUrl="https://privacy.microsoft.com/privacystatement">Privacy</LinkItem>
<LinkItem targetUrl="https://privacy.microsoft.com/privacystatement">
Privacy and cookies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit : 'Privacy Policy' is the common term used for these kinds of links

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw this requirement in the Privacy review, will confirm with Jade!

@@ -14,6 +31,7 @@ import { Homepage } from "@site/src/components/home";
export default function Home(): React.ReactElement {
return (
<Layout>
<Suspense fallback={<div />}></Suspense>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any loading element we can use here other than just an empty div?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we only expect this to appear during local development, leaving an explicit notice with more details sounds like a good idea to me.

"InstrumentationKey=8bce9107-e3f7-445c-be87-153f7ecbbe47;IngestionEndpoint=https://centralus-2.in.applicationinsights.azure.com/;LiveEndpoint=https://centralus.livediagnostics.monitor.azure.com/;ApplicationId=11a3a32b-28c1-4f28-97fb-4b113a61c1e9",
enableAutoRouteTracking: true,
enableDebug: true,
extensions: [reactPlugin],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the react plugin here looks to give you component level tracking but it looks like you need to use a higher order component for it to work, for example export default withAITracking(reactPlugin, MyComponent); (see https://www.npmjs.com/package/@microsoft/applicationinsights-react-js)

I'm not sure if enabling this plugin does anything without the use of this higher order component?

@@ -4,3 +4,13 @@ strict-peer-dependencies=true

# Disable pnpm update notifications since we use corepack to install package managers
update-notifier=false

@wcp:registry=https://1essharedassets.pkgs.visualstudio.com/1esPkgs/_packaging/WebCompliance/npm/registry/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a user tries to install without the necessary environment variables set?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does process.env.<value> even work here? I thought this was more of a static file, not something that gets executed in a node context. If that's true, what we need is probably docs saying "in order to work with @wcp/wcp-consent you need to add to your .npmrc file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Josmithr They would get a 401 which would fail gracefully, since the package is in optionalDependencies.

@alexvy86 You're right .npmrc is static. I think you can do export VAR=foo and then interpolate it using ${} but adding to .npmrc is the right convention

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't know you could do that.

docs/README.md Outdated Show resolved Hide resolved
@@ -104,5 +105,12 @@
"packageManager": "[email protected]+sha512.d1a029e1a447ad90bc96cd58b0fad486d2993d531856396f7babf2d83eb1823bb83c5a3d0fc18f675b2d10321d49eb161fece36fe8134aa5823ecd215feed392",
"engines": {
"node": ">=18.0"
},
"dependencies": {
"@microsoft/applicationinsights-react-js": "^17.3.4",
Copy link
Contributor

@Josmithr Josmithr Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should move under devDependencies (since this doesn't get published as a library, all of the deps can be dev).

docs/package.json Outdated Show resolved Hide resolved
docs/README.md Outdated
## Optional Packages

If you would like to include consent management functionality in your local development environment,
you can install the `@wcp/wcp-consent` package. This package is used for handling user consent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend linking to documentation for this library, and then note that it's MS-internal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I'd write all this from the perspective of "This bit is related to the production deployment of the site; if you're doing local development it's probably not relevant to you. If you're a Microsoft engineer who needs to work on this, refer to our internal documentation."

On related topics, using optional-dependencies does seem to work. If our pipelines set up the appropriate npm registries and credentials, the package will get installed when running there, and it'll just be ignored when installing the deps locally.

docs/README.md Outdated Show resolved Hide resolved
docs/docusaurus.config.ts Outdated Show resolved Hide resolved
docs/src/pages/index.tsx Outdated Show resolved Hide resolved
docs/src/pages/index.tsx Outdated Show resolved Hide resolved
docs/src/pages/index.tsx Outdated Show resolved Hide resolved
@WayneFerrao WayneFerrao marked this pull request as ready for review December 17, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: website base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants