Add Google Analytics #1049

apedroferreira · 2022-09-23T17:58:55Z

Closes #997

I think this basic integration should cover all the data we need for now.

render · 2022-09-23T17:58:59Z

Your Render PR Server URL is https://toolpad-pr-1049.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-ccmv7ocgqg42l9btord0.

bytasv

Wondering if it made sense to use something like https://www.npmjs.com/package/nextjs-google-analytics ? - if I understood correctly we get pretty much the same functionality there

apedroferreira · 2022-09-27T14:36:30Z

Wondering if it made sense to use something like https://www.npmjs.com/package/nextjs-google-analytics ? - if I understood correctly we get pretty much the same functionality there

I didn't know about this library, it doesn't seem very popular, maybe because it really isn't that much simpler/ easier to use than this integration, and does not provide any meaningful advantages?
Also I would argue that not relying on another external dependency gives us a bit more control and knowledge about what our integration does? Plus that way we don't have to rely on that dependency being actively mantained.

bytasv · 2022-09-27T15:41:47Z

Wondering if it made sense to use something like https://www.npmjs.com/package/nextjs-google-analytics ? - if I understood correctly we get pretty much the same functionality there

I didn't know about this library, it doesn't seem very popular, maybe because it really isn't that much simpler/ easier to use than this integration, and does not provide any meaningful advantages? Also I would argue that not relying on another external dependency gives us a bit more control and knowledge about what our integration does? Plus that way we don't have to rely on that dependency being actively mantained.

That's fair. I just looked around and this popped up, I don't know about the popularity, but seems that it is somewhat actively maintained and from the initial impression it already does all what you had to implement so my thinking was that we might be just able to use that out of the box.
As for more control and knowledge about it - maybe you could be a little bit more specific? For me it would be enough to know that whatever the library is it does what I want it to do and if I start seeing some issues with it like maybe lack of functionality, that's then I would consider switching to home-made solution. Anyways these are just 2 cents from me and how I ended up there

apedroferreira · 2022-09-27T15:59:38Z

That's fair. I just looked around and this popped up, I don't know about the popularity, but seems that it is somewhat actively maintained and from the initial impression it already does all what you had to implement so my thinking was that we might be just able to use that out of the box. As for more control and knowledge about it - maybe you could be a little bit more specific? For me it would be enough to know that whatever the library is it does what I want it to do and if I start seeing some issues with it like maybe lack of functionality, that's then I would consider switching to home-made solution. Anyways these are just 2 cents from me and how I ended up there

This implementation I made isn't too complex in my opinion, just analytics + page tracking + web vitals (not that web vitals are important for now, but they were just easy to add), so I don't think we'll have any issues maintaining it either.
It's not that the library you proposed has any big disadvantages, but as I've already implemented things this way I think we could keep it, as I don't see any advantages in using the library either, and there would be the effort in switching over.

The kind of issues that could come from using the library isn't too probable in the short-term but would be something like:

Google Analytics makes changes to their API
The library does not get updated / stops being actively maintained
We're stuck with the library unless we change to a manual implementation

bytasv · 2022-09-27T17:01:10Z

As I mentioned that's fair enough, I'm not pushing to replace implementation, let's keep it as is, I just wanted to share how and why I was thinking of a library in the first place.
While your mentioned concerns can be valid in the situations that you described if the lib did what we need to do and it worked right now, none of that would matter and in the worse case scenario we could switch to home-made solution.
IMO this is something that we could/should keep an eye for in the future when we need to implement very common functionality and ask our selves whether we really want to write and maintain code ourselves (ideally that code should also be tested) if we could use something of the shelf (supposingly we could). I think it could sometimes be deceptive at first to treat that this is the only functionality that we would need for such thing and find ourselves adding more and more features just to end up recreating the same thing in house.

apedroferreira · 2022-09-27T17:04:15Z

As I mentioned that's fair enough, I'm not pushing to replace implementation, let's keep it as is, I just wanted to share how and why I was thinking of a library in the first place. While your mentioned concerns can be valid in the situations that you described if the lib did what we need to do and it worked right now, none of that would matter and in the worse case scenario we could switch to home-made solution. IMO this is something that we could/should keep an eye for in the future when we need to implement very common functionality and ask our selves whether we really want to write and maintain code ourselves (ideally that code should also be tested) if we could use something of the shelf (supposingly we could). I think it could sometimes be deceptive at first to treat that this is the only functionality that we would need for such thing and find ourselves adding more and more features just to end up recreating the same thing in house.

Makes sense, let's keep an eye out for that sort of thing in the future.
Thanks for the feedback!

Janpot · 2022-10-04T15:06:04Z

packages/toolpad-app/next.config.mjs

@@ -167,6 +167,6 @@ export default /** @type {import('next').NextConfig} */ ({
 source: '/:path*',
 headers: securityHeaders,
 },
- ]


@apedroferreira Any idea on how this passed the prettier CI check up until now?

Same with docs/next.config.js. It gets updated when I run yarn prettier:all

not really, maybe it's not running for files with the .mjs extension?
i probably just saved the file and prettier added this

weird... if i run it locally this file always gets fixed

Did prettier get updated recently? CI only checks for changed files I believe. Perhaps we should run prettier:all in CI?

ah, i merged this from renovatebot 735011e

that was probably it

Janpot · 2022-10-04T15:17:24Z

packages/toolpad-app/pages/_app.tsx

+
+ const router = useRouter();
+
+ React.useEffect(() => {


@apedroferreira

How useful would this really be? We also have react-router on the editor, so the data wouldn't be exactly a good representation of user navigation throughout the app.

Did you verify we only send one event on hard reload?

I took it from this example https://github.com/vercel/next.js/tree/canary/examples/with-google-analytics, just rechecked again and as far as I can tell:

hard reload only triggers one page view

client side router changes (creating a new connection for example) trigger page views too, probably because of hashChangeComplete

seems to run for the iframe routes too, not sure if that's helpful

might make sense to disable for the app canvas

ok actually it turns out this effect is doing nothing at all... the page view events are being sent automatically.
i'll remove this effect and see if i can filter out events from the iframe

and because the page view events are sent automatically doesn't seem simple to filter out the ones from the canvas... - we can just ignore that data in the google analytics dashboard itself if we want to use page view data, im gonna make a PR that just removes the effect

Add Google Analytics (no events yet)

5a2f6dc

apedroferreira self-assigned this Sep 23, 2022

Better names for env variable

6a5dc4a

apedroferreira marked this pull request as ready for review September 26, 2022 15:22

apedroferreira requested a review from a team September 26, 2022 15:22

Add checks in GA methods

bfbf48c

bytasv reviewed Sep 27, 2022

View reviewed changes

bytasv approved these changes Sep 27, 2022

View reviewed changes

apedroferreira merged commit 070cfda into master Sep 27, 2022

apedroferreira deleted the add-ga branch September 27, 2022 17:04

Janpot reviewed Oct 4, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Google Analytics #1049

Add Google Analytics #1049

apedroferreira commented Sep 23, 2022 •

edited

Loading

render bot commented Sep 23, 2022

bytasv left a comment

apedroferreira commented Sep 27, 2022 •

edited

Loading

bytasv commented Sep 27, 2022

apedroferreira commented Sep 27, 2022

bytasv commented Sep 27, 2022

apedroferreira commented Sep 27, 2022

Janpot Oct 4, 2022

Janpot Oct 4, 2022

apedroferreira Oct 4, 2022 •

edited

Loading

apedroferreira Oct 4, 2022

Janpot Oct 4, 2022

apedroferreira Oct 4, 2022

Janpot Oct 4, 2022

apedroferreira Oct 4, 2022 •

edited

Loading

apedroferreira Oct 4, 2022 •

edited

Loading

Janpot Oct 4, 2022

apedroferreira Oct 4, 2022

apedroferreira Oct 4, 2022

Add Google Analytics #1049

Add Google Analytics #1049

Conversation

apedroferreira commented Sep 23, 2022 • edited Loading

render bot commented Sep 23, 2022

bytasv left a comment

Choose a reason for hiding this comment

apedroferreira commented Sep 27, 2022 • edited Loading

bytasv commented Sep 27, 2022

apedroferreira commented Sep 27, 2022

bytasv commented Sep 27, 2022

apedroferreira commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira Oct 4, 2022 • 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

apedroferreira Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

apedroferreira Oct 4, 2022 • 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

apedroferreira commented Sep 23, 2022 •

edited

Loading

apedroferreira commented Sep 27, 2022 •

edited

Loading

apedroferreira Oct 4, 2022 •

edited

Loading

apedroferreira Oct 4, 2022 •

edited

Loading

apedroferreira Oct 4, 2022 •

edited

Loading