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

[AIRFLOW-1498] Add analytics script #5850

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Conversation

ryw
Copy link
Member

@ryw ryw commented Aug 18, 2019

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

We want to collect anonymous user event data from our Airflow deployments, without having to maintain a fork or plugin. This PR adds an easy method for Airflow users to send anonymous user event data to several popular destinations: Google Analytics, Segment, and Metarouter.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Didn't any relevant tests to change, but I could be missing something as I'm new to contributing to Airflow. Happy to write some tests, if it is warranted.

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

@schnie
Copy link
Contributor

schnie commented Aug 18, 2019

@ryw curious, why did we switch from being able to specify any analytics snippet to the predefined blocks for each tool?

@ryw
Copy link
Member Author

ryw commented Aug 18, 2019

Primarily because of the user experience of being required to that.

When I started writing the doc for the other idea, I realized how user unfriendly it was, to force them to develop and deploy a script to another URL, and I think there would be cross site scripting issues to deal with there as well.

With the solution here, we are implementing the JavaScript library in the way that the tool author specified by dropping the JavaScript snippets directly into the page.

airflow/www/templates/appbuilder/baselayout.html Outdated Show resolved Hide resolved
docs/howto/tracking-user-activity.rst Outdated Show resolved Hide resolved
docs/howto/tracking-user-activity.rst Outdated Show resolved Hide resolved
airflow/config_templates/default_airflow.cfg Outdated Show resolved Hide resolved
@ryw
Copy link
Member Author

ryw commented Aug 19, 2019

Just pushed updated version, wondering if the analytics partials should go in new folder (like I did) or inside one of the other two folders. Both felt off.

@ryw
Copy link
Member Author

ryw commented Aug 19, 2019

@potiuk or @ashb any thoughts on the Travis fail for this https://travis-ci.org/apache/airflow/jobs/573878437?

.pre-commit-config.yaml doesn't have a section for the rst Add licence for all rst file

And the .rst file that I added does have the license

@potiuk
Copy link
Member

potiuk commented Aug 20, 2019

Hey @ryw -> I am going to add a --show-diff-on-failure flag shortly to show more details about the failure so that it will be obvious. Sorry for any inconvenience.

In the meantime you can run 'pre-commit run insert-license` (see https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pre-commit-hooks-installed) - it's not yet 100% clear in the master docs but some more instructions are coming.

The .pre-commit-confg.yaml does have the section:

      - id: insert-license
        name: Add licence for all rst files
        exclude: ^\.github/.*$"|^airflow/_vendor/.*$
        args:
          - --comment-style
          - "||"
          - --license-filepath
          - license-templates/LICENSE.rst
        files: \.rst$

In this case it's probably spacing issue. Due to limitation of insert-license pre-commit we have to have a space in front of the .. in the .rst license. This has been changed recently in all the files while I committed the change so you probably took the license from previous (no-space-in-front) version. You can take a look at any of the .rst files now in master and copy&paste the license from there. This is what most people will do in the future I guess, so this will not be a problem.

And BTW if you install pre-commit hooks yourself, you will not need to worry about the licences at all. When you try to commit the file without the license, the license will be added for you, commit will fail and you will have a chance to add/commit the added licence immediately.

So you can just remove the licence now, install pre-commit, try to commit it and the licence will be added for you automagically.

Sorry for the inconvenience coming from the transition period.

@ryw
Copy link
Member Author

ryw commented Aug 20, 2019

Ah thanks @potiuk - I now see you just added yesterday, my branch was 2 days out of date w/ master :) Just pushed up new version, should pass now.

@ryw
Copy link
Member Author

ryw commented Aug 21, 2019

Green! 🍏

@ashb ashb merged commit 28e3802 into apache:master Aug 22, 2019
@ashb ashb deleted the AIRFLOW-1498 branch August 22, 2019 11:32
ashb pushed a commit that referenced this pull request Aug 22, 2019
`Google Analytics <https://analytics.google.com/>`_,
`Segment <https://segment.com/>`_, or `Metarouter <https://www.metarouter.io/>`_.

Edit ``airflow.cfg`` and set the ``analytics`` block to have a ``tool`` and ``id``:
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that the analytics block does not exist. Did I miss something?

Copy link
Member Author

@ryw ryw Aug 22, 2019

Choose a reason for hiding this comment

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

Doh! We moved it to webserver block and changed var name. Will send doc fix PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

5 participants