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 plausible analytics #828

Merged
merged 13 commits into from Jul 24, 2022
Merged

Add plausible analytics #828

merged 13 commits into from Jul 24, 2022

Conversation

tupui
Copy link
Contributor

@tupui tupui commented Jul 20, 2022

Closes #827

This adds Plausible analytics. I am not sure about what I did, but I think I edited all the files which needed to be edited 😅

cc @choldgraf @stefanv

@tupui
Copy link
Contributor Author

tupui commented Jul 20, 2022

So I am not sure what I am missing but this is failing:

E         + WARNING: unsupported theme option 'plausible_domain' given
[73](https://github.com/pydata/pydata-sphinx-theme/runs/7438329801?check_suite_focus=true#step:7:74)
E         + WARNING: unsupported theme option 'plausible_url' given

Copy link

@stefanv stefanv 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 to me @tupui! Some minor language tweaks in the docs.

docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
@choldgraf
Copy link
Collaborator

A quick thought on your error message. It looks like in your examples you're using analytics_domain but in theme.conf you're using plausible_analytics_domain. I think that's probably the error.

Can I get a quick gut-check on whether it'd be more sensible to nest these under a single analytics configuration value, so that we don't create new Sphinx configuration values each time we want to extend analytics functionality? e.g.:

html_theme_options = {
  "analytics": {"plausiable__analytics_domain": "foo", "plausible__analytics_url": "bar"},
}

or

html_theme_options = {
  "analytics": {"google_analytics_id": "foo"},
}

or is that over-engineering API? 😅

@tupui
Copy link
Contributor Author

tupui commented Jul 21, 2022

Thank you both for you reviews!

@stefanv I rephrased the doc. Let me know how it reads.

@choldgraf indeed! Sometimes... 😅 Let see now. For the configuration as you wish. I do think that what you propose would look cleaner, but then we need to think about backward compatibility. LMK if I should make the change. I would then raise a deprecation warning in case the other syntax is used.

I am looking at the tests. I will try to run these in debug locally. Not sure what I am doing there.

@tupui
Copy link
Contributor Author

tupui commented Jul 21, 2022

Ok I think I fixed it. This is adding the following, which I believe is correct:

 <script defer="defer">
             data-domain=toto src=http://.../script.js
         </script>,

@choldgraf
Copy link
Collaborator

Personally, I think it's be cleaner if we just defined a top level "analytics" config value. We already have large-ish top-level config surface area so this might help keep things a bit ordered. +1 on a deprecation cycle for the current Google analytics IDs. I don't feel strongly about it though, so if you or others disagree that it's a good idea, we can skip it. What do you think? 🙂

@tupui
Copy link
Contributor Author

tupui commented Jul 21, 2022

I also prefer to not grow the API, I made the change. (I just did not add a test to catch the warning itself. Not sure we really have to. LMK)

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me - I'm +1 after my minor comments are resolved.

docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
all html pages to gather metrics. And the dashboard can be accessed at
``https://url/my_domain``.

The Scientific-Python community can offer a self-hosted server. Contact the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this documented somewhere? We were looking into Plausible for Jupyter a while back, but didn't do it in-part because it was pretty expensive to run at the scale of Jupyter's website. I wonder if we could piggy-back on scientific-python with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documented as what is the process to get access? Nothing yet, this would be the first document talking about that. At some point we might have some mention on our website when we list all the initiatives we have.

@stefanv maybe you can answer this one? I don't know how hard this is for the server.
But I would guess it's ok. SciPy is getting 100k per month which is nothing. I don't know how many RPS the server can handle but I would assume >100 RPS (based on my personal experience, we should see this with e.g. locust to be sure)

docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Show resolved Hide resolved
src/pydata_sphinx_theme/__init__.py Show resolved Hide resolved
src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
tests/test_build.py Outdated Show resolved Hide resolved
tests/test_build.py Show resolved Hide resolved
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Copy link
Contributor Author

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thank you for the review and suggestions Chris! I will work on that.

tests/test_build.py Show resolved Hide resolved
src/pydata_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
all html pages to gather metrics. And the dashboard can be accessed at
``https://url/my_domain``.

The Scientific-Python community can offer a self-hosted server. Contact the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documented as what is the process to get access? Nothing yet, this would be the first document talking about that. At some point we might have some mention on our website when we list all the initiatives we have.

@stefanv maybe you can answer this one? I don't know how hard this is for the server.
But I would guess it's ok. SciPy is getting 100k per month which is nothing. I don't know how many RPS the server can handle but I would assume >100 RPS (based on my personal experience, we should see this with e.g. locust to be sure)

@tupui
Copy link
Contributor Author

tupui commented Jul 22, 2022

Alright, I did the changes 😃

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

this looks pretty good to me! I have a few comments in there with suggestions and questions, let me know what you think!

docs/user_guide/analytics.rst Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Outdated Show resolved Hide resolved
docs/user_guide/analytics.rst Show resolved Hide resolved
src/pydata_sphinx_theme/__init__.py Show resolved Hide resolved
"""

# Link the JS files
app.add_js_file(gid_js_path, loading_method="async")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason we'd use defer for plausible, but async here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this but apparently this is what they have in their doc. Maybe to have correct timing?? https://plausible.io/docs/plausible-script

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Copy link
Contributor Author

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thank you for the suggestions!

"""

# Link the JS files
app.add_js_file(gid_js_path, loading_method="async")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this but apparently this is what they have in their doc. Maybe to have correct timing?? https://plausible.io/docs/plausible-script

@choldgraf choldgraf merged commit 75c7754 into pydata:main Jul 24, 2022
@choldgraf
Copy link
Collaborator

Thanks for the contribution! This looks good to me, let's see if there are some edge-cases to debug in the future

@tupui
Copy link
Contributor Author

tupui commented Jul 24, 2022

Great thanks Chris!


If your documentation is for a package that is part of the SciPy / PyData
ecosystem, they might be able to host a Plausible server for you at
``<your-package>.scientific-python.org``.
Copy link

Choose a reason for hiding this comment

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

@tupui Note that we host all projects at views.scientific-python.org

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there anything project-specific in a URL? Or just one big dashboard for everybody?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes it should be https://views.scientific-python.org/<your-package>

- A URL pointing to the JavaScript analytics script that is served by your Plausible server
- A domain that reflects where your documentation lives

Plausible' javascript will be included in all html pages to gather metrics.
Copy link

Choose a reason for hiding this comment

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

Incorrect usage of apostrophe

ga('set', 'anonymizeIp', true);
ga('send', 'pageview');
msg = (
"'google_analytics_id' is deprecated and will be removed in "
Copy link

Choose a reason for hiding this comment

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

Does this not affect the current Google Analytics section in the documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm I did not update this indeed.

Copy link
Contributor Author

@tupui tupui Jul 25, 2022

Choose a reason for hiding this comment

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

Correction, I mis-read that and I did update the doc about Google Analytics. Ah no I see what you meant. Chris did it right in the follow-up

Copy link
Contributor Author

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Thanks @stefanv I will make a follow up.


If your documentation is for a package that is part of the SciPy / PyData
ecosystem, they might be able to host a Plausible server for you at
``<your-package>.scientific-python.org``.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes it should be https://views.scientific-python.org/<your-package>

ga('set', 'anonymizeIp', true);
ga('send', 'pageview');
msg = (
"'google_analytics_id' is deprecated and will be removed in "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm I did not update this indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Plausible analytics
4 participants