jazzband/django-analytical

Hard to use in development environment

garrettr opened this issue · 13 comments

I just used django-analytical to integrate Piwik analytics into a site, using the Piwik-specific template tag as documented here. It works great in production, but unfortunately it causes problems for our development environment.

Obviously we don't want to track visits to the site when it's running on a local development server. It seems like it would be best to simply not embed the tracking code in the templates when we're running in development. Furthermore, it would nice to not be required to set PIWIK_DOMAIN_PATH or PIWIK_SITE_ID in the development settings, since they are unnecessary.

The documentation on PIWIK_SITE_ID says:

If you do not set the site ID the tracking code will not be rendered.

This implies that the {% piwik %} template tag won't render the tracking code if PIWIK_SITE_ID is not set, which would be very desirable behavior for the use case of the development environment. In that case, you would only have to set PIWIK_SITE_ID in settings/production.py to get the Piwik code to render in production, and it would not be rendered in the development environment.

Unfortunately, this part of the documentation appears to be incorrect. Failing to set PIWIK_DOMAIN_PATH or PIWIK_SITE_ID raises an AnalyticalException, which breaks template rendering in the development environment.

For our project, this necessitated an annoying workaround: we set PIWIK_SITE_ID to 0 (an invalid value) in settings/development.py, pass that setting to the template context via a context processor, and conditionally render the Piwik template tag based on the value of PIWIK_SITE_ID. We can't even set the PIWIK_* values to None, which would be more explicit and therefore preferable, because of the validation in get_required_setting.

It would be better for the documented behavior to be implemented, because that would make django-analytical work simply in both production and development environments, and would not require the aforementioned workaround. Another option is to simply correct the documentation, although that would leave the annoying behavior in place and still necessitate a workaround.

Am I missing anything here? Are there best practices for using django-analytical in a development environment that I am unaware of?

We should fix the implementation. Development should be easy, ideally intuitive.

I think you should use the INTERNAL_IPS settings property for development. This feature is covered by the tests, so it should work reliably. See Django's Settings documentation for details on INTERNAL_IPS, the default value for ANALYTICAL_INTERNAL_IPS.

The sentence "If you do not set the site ID the tracking code will not be rendered" in the documentation should be removed as SITE_ID is a required setting. I fear it would be confusing to make it not being rendered when the SITE_ID is empty---which could be by error. -- Sorry for the mistake in the docs.

I just discovered that the generic tags ({% analytical_head_top %}, etc.) work differently than the specific tags (e.g. {% piwik %}). Unlike the specific tags, which fail with an error if the corresponding configuration is unavailable or invalid, the generic tags fail silently if there are no analytics services configured in the settings. I think the generic tags' behavior is better, and makes it easy to use django-analytical in development, staging, and production environments.

I have just noticed the same problem as @garrettr . INTERNAL_IPS does not solve the problem - there should be some settings variable disabling the Piwik tag.

What INTERNAL_IPS scenario is missing is that when one has the dev server running on some machine and a team of developers/testers sending the requests from their local machines or mobile devices one would have to register all that devices IPs in the '[ANALYTICAL_]INTERNAL_IPS` var. This should not be the desired behavior.

I think, the very simple solution for this could be:

  • from django.conf import settings in analytical/templatetags/piwik.py
  • and then update the PiwikNode.render method:
if is_internal_ip(context, 'PIWIK') or settings.DEBUG:
            html = disable_html(html, 'Piwik')

I would however, opt for some more explicit settings variable like ANALYTICAL_ENABLED (=True by default). This (or similar) solution should be applied to all the other tags related to other analytical services.

What do you think about it @bittner

@sebzur Difficult to say. A new setting to disable analytical globally may be practical. Though, I'm not a big fan of polluting the code with additional settings. "There should be one-- and preferably only one --obvious way to do it." (PEP 20)

The beauty of INTERNAL_IPS is that this is a functionality provided and used by the Django framework itself. I've not seen any hints in the documentation whether specifying IP ranges is possible, which may solve the "may IPs to add" issue; it seems not to be.

@garrettr If the generic and specific tags behave differently, that's a problem. -- Do they really? Why? @jcassee, can you tell?

I'm not a big fan of silent errors. An erroneous application should fail to tell us we need to fix a problem. "Errors should never pass silently. Unless explicitly silenced." (PEP 20)

Could you not simply check a setting in your base template(s)? For example:
{% if not settings.DEBUG %}{% analytical_body_bottom %}{% endif %}

We're using google analytics. Normally when we don't want something to run we would use

{% if not debug %} ...stuff... {% endif %}

However, that doesn't appear to be working when using this plugin like so:

{% if not debug %}
    {% google_analytics %}
{% endif %}

doesn't prevent the plugin from loading and attempting to find the ga variable, producing:

AnalyticalException at / GOOGLE_ANALYTICS_PROPERTY_ID setting is not set

Why is the plugin still rendering? That is quite absurd.

However, using:

{% if not debug %}
    {% analytical_head_top %}{% analytical_head_bottom %}
{% endif %}

does return the expected behavior. Why is this any different? I'd prefer to stick with just the google analytics tag since it's the only library we import.

Sorry if this is not 100% the answer you want. If there is a bug or irritating behavior this deserves to be fixed. I agree. Anyway, here are my two cents on your side comment:

I'd prefer to stick with just the google analytics tag since it's the only library we import.

If you restrict yourself to a single provider what is the benefit of this? Does your code get any more readable? Do you have any performance benefits? Those are the questions you should ask yourself.

And if one day the unlikely event occurs that you actually want to switch to another analytics provider, hey, then you get all the actual, real benefits of this Python package. Think about it. I would advise on actually using this package for the reason it was designed for.

The exception you mention above is raised at analytical.utils, line 25, which in your case probably comes from templatetags.google_analytics, line 82 via line 77 in the same file.

I would guess you don't have the tag loading enclosed in an if-clause on top of your template.

That's a fair enough response, and using the generic import is doing the trick for us. I suppose that I just expected it to behave the same way. Since it doesn't behave the same way, you might consider adding some documentation about the caveats of using the individual imports.

Mind to make a PR for this?

BTW, as I suspected, the reason why the generic template tag works and the single one throws an exception is such that the generic tag ignores exceptions that happen in the single implementations. Exceptions that occur in the contribute_to_analytical function pass silently (for a valid reason) this way.

When you do things "the manual way" you also have to make sure yourself that either the exception is silenced or, better, all preconditions for the code being successfully executed (or not being executed at all!) must be met. As I pointed out earlier you probably had a {% load google_analytics %} in your template that was not covered by any {% if not debug %} clause, and according to the exception you don't seem to set the GOOGLE_ANALYTICS_PROPERTY_ID in your Django settings when debug is True.

If this is all true then there's little to document. Then the implementation behaves as we should expect.