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

Snippet integration verification #4106

Merged
merged 39 commits into from
May 23, 2024
Merged

Snippet integration verification #4106

merged 39 commits into from
May 23, 2024

Conversation

aerosol
Copy link
Member

@aerosol aerosol commented May 15, 2024

Changes

This PR introduces a new feature - snippet integration verification.
The feature is exposed in two places:

  • during standard onboarding (site creation)
  • in the General Settings tab, for manual execution
record-2024-05-21-15-43-00-fled.jowl.mats.mp4

Note for reviewers

This PR is rather sizeable - reviewing commit by commit should make it bearable.
To play with it locally end to end, one has to make browserless - this will instantiate a self-hosted browserless.io docker image.
Please look at the tests first when in doubt.

How it works

Several modules implementing Plausible.Verification.Check run in a pipeline, modifying Plausible.Verification.State.
The state is later on interpreted into Plausible.Verification.Diagnostics.Rating, that carries the final result with, in case of failure, associated user-facing error and troubleshooting recommendation.

At a higher level, this is coordinated by Plausible.Live.Verification LiveView, that can be either embedded standalone (onboarding) or as in a modal (settings). There are slight differences for each mode. For instance, onboarding verification still keeps waiting for the first organic page-view (but lets the user skip to the dashboard however).

In general the following checks are made:

  • retrieving, parsing and inspecting the site's HTTP response server-side
  • running a headless browser script, verifying if plausible() function is defined at the target site, sending a test verification event to the server, and checking ingestion response status

A bit more detailed descriptions can be found in each check's moduledoc.
The headless browser integration is done via browserless.io function REST API.

The most critical piece is Diagnostics.Rating - we try to make it as accurate as possible, in order to help people troubleshoot their integrations without having to go through customer support. Tests should capture most of the scenarios we initially want to support.

Feature flag

We'll deploy it behind :verification flag initially. Alternatively, verification can be enabled permanently by setting VERIFICATION_ENABLED=true environment variable. That variable is on by default for dev environment, off otherwise.

Documentation updates

Documentation/troubleshooting pages will require an update - pending.

Self-hosting

cc @ruslandoga - by default the feature is off and should not interfere with self-hosted deployments in any way.
We can later on add documentation on how to deploy it along with self-hosted browserless.io, if need be.

Gotchas

⚠️ This PR modifies the plausible tracker. For that reason, in order to run it end to end, we must make sure that the snippet refers to the updated tracker. Otherwise, the error messages from diagnostic rating can get rather confusing.

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@aerosol aerosol added the deploy-to-staging Special label that triggers a deploy to a staging environment label May 15, 2024
@aerosol aerosol force-pushed the browserless-stack branch 5 times, most recently from 858a717 to 4aefdb4 Compare May 21, 2024 11:04
@aerosol aerosol requested a review from a team May 21, 2024 13:45
@aerosol aerosol changed the title Browserless stack wip Site verification May 21, 2024
@aerosol aerosol changed the title Site verification Snippet integration verification May 21, 2024
aerosol added 21 commits May 22, 2024 08:46
- Floki will be used on production to parse site contents
- Req will be used to handle redundant stuff like retrying etc.
Later on we'll use it to indicate verification errors
1. headless browsers aren't ignored if `window.__plausible` is defined
2. callback optionally supplies the event response HTTP status

This will be later used to check whether the server acknowledged
the verification event.
It:
 - tries to visit the site
 - defines window.__plausible, so the tracker doesn't ignore test events
 - sends a verification event and instruments the callback
 - awaits the callback to fire and returns the result
Only report CSP error if the snippet is already found
Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

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

Looks good in general. I would like to play around with the UI as well but I don't think I can get to it today and I'm off for the rest of the week. Don't let it block this from being released though.

lib/plausible/verification/checks.ex Outdated Show resolved Hide resolved
lib/plausible/verification/checks/csp.ex Show resolved Hide resolved
lib/plausible/verification/diagnostics.ex Show resolved Hide resolved
@cnkk cnkk added the preview label May 22, 2024
@@ -80,7 +81,8 @@ defmodule PlausibleWeb.StatsController do
!stats_start_date && can_see_stats? ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
!stats_start_date && can_see_stats? ->
can_see_stats? and not has_stats? ->

as a follow-up to previous suggestion

lib/plausible/verification/check.ex Outdated Show resolved Hide resolved
lib/plausible/verification/check.ex Outdated Show resolved Hide resolved
lib/plausible/verification/checks/installation.ex Outdated Show resolved Hide resolved
lib/plausible/verification.ex Outdated Show resolved Hide resolved
test/test_helper.exs Show resolved Hide resolved
lib/plausible/verification/checks/snippet_cache_bust.ex Outdated Show resolved Hide resolved
test/plausible/site/verification/checks_test.exs Outdated Show resolved Hide resolved
Copy link
Contributor

@zoldar zoldar left a comment

Choose a reason for hiding this comment

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

Part of the review comments I've made are actually addressed in further commits from the ones I was reviewing at the time - please ignore those. That aside, there are only really some tiny or almost cosmetic nits.

All that aside, awesome work ✨

@zoldar
Copy link
Contributor

zoldar commented May 23, 2024

Oh one more thing that just came to my mind - running browserless in detached mode per default, explicitly named with an easy way to stop it (following example of minio setup).

diff --git a/Makefile b/Makefile
index a85ee3b8f..527711a8d 100644
--- a/Makefile
+++ b/Makefile
@@ -38,7 +38,10 @@ postgres-stop: ## Stop and remove the postgres container
        docker stop plausible_db && docker rm plausible_db
 
 browserless:
-       docker run -e "TOKEN=dummy_token" -p 3000:3000 --network host ghcr.io/browserless/chromium
+       docker run -d --rm -e "TOKEN=dummy_token" -p 3000:3000 --name plausible_browserless --network host ghcr.io/browserless/chromium
+
+browserless-stop:
+       docker stop plausible_browserless

WDYT?

aerosol and others added 4 commits May 23, 2024 13:44
Co-authored-by: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Co-authored-by: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
Co-authored-by: Adrian Gruntkowski <adrian.gruntkowski@gmail.com>
@aerosol
Copy link
Member Author

aerosol commented May 23, 2024

Oh one more thing that just came to my mind - running browserless in detached mode per default, explicitly named with an easy way to stop it (following example of minio setup).

Will do that in a follow-up maybe. For now it's quite convienent to be able to see the logs attached and being able to stop it with C-c.

@aerosol aerosol merged commit c81cb16 into master May 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy-to-staging Special label that triggers a deploy to a staging environment preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants