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

Telemetry Sender #26

Merged
merged 4 commits into from
Jul 2, 2015
Merged

Telemetry Sender #26

merged 4 commits into from
Jul 2, 2015

Conversation

akx
Copy link
Contributor

@akx akx commented Jun 23, 2015

Implements an opt-in-by-default telemetry sender.

The receiving side is still missingcurrently in PR review, so this doesn't do much except slow down loading of the admin dashboard for a few seconds every 72 hours.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/730/
Test FAILed.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/731/
Test PASSed.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/732/
Test PASSed.

@Pikkupomo
Copy link
Contributor

I think we should say more clearly that this is happening. Currently only way of knowing this is by navigating into TelemetryView.

If we want to send 1 telemetry anyways, we should then (in dashboard) say that Shoop is collecting telemetry from installation, see <link> for more information and a possibility to opt-out if you want. Or by directing there after first access to dashboard.

Currently the feeling is that its happening behind the curtains without user ever noticing it. I think we should push this more towards these 2 facts:

  • User understands that this is happening
  • We have his permission to do so

Otherwise looks good 👍

@akx
Copy link
Contributor Author

akx commented Jun 25, 2015

You're absolutely correct: one telemetry payload is sent unless the user knows to navigate to /sa/system/telemetry/ before visiting the dashboard to opt out of the feature.

I'm not sure I agree with the procedure either, though I reasoned with myself that it isn't that bad, considering it contains no business information. But yeah, you're right, this is definitely happening very quietly, even sneakily, behind the curtains.

I also have the feeling giving users the choice to opt-out by default would have more users opting out than leaving it be, and I got the impression from @tomiap that we really do want to gather these statistics anyway.

For the record, here's an example payload (as displayed on the Telemetry information page):

{
  "apps": [
    "django.contrib.admin",
    "django.contrib.auth",
    "django.contrib.contenttypes",
    "django.contrib.sessions",
    "django.contrib.messages",
    "django.contrib.staticfiles",
    "django_jinja",
    "filer",
    "easy_thumbnails",
    "shoop.core",
    "shoop.simple_pricing",
    "shoop.simple_supplier",
    "shoop.default_tax",
    "shoop.front",
    "shoop.front.apps.registration",
    "registration",
    "shoop.front.apps.auth",
    "shoop.front.apps.customer_information",
    "shoop.front.apps.personal_order_history",
    "shoop.front.apps.simple_order_notification",
    "shoop.front.apps.simple_search",
    "shoop.admin",
    "shoop.addons",
    "shoop.testing",
    "bootstrap3",
    "shoop.notify",
    "shoop.simple_cms"
  ],
  "host": "127.0.0.1:8000",
  "key": "icbTWzYGQb9UT4OteMGioIAsjrUabx2GbuHlnldRwwSq6wjo",
  "machine": "AMD64",
  "platform": "Windows-8-6.2.9200",
  "python_version": "3.4.2 (v3.4.2:ab2c023a9432, Oct  6 2014, 22:16:31) [MSC v.1600 64 bit (AMD64)]",
  "shoop_version": "1.0.0"
}

So yeah -- this really is a policy issue. I'm happy to rework the code to be opt-in by default, or something else altogether, but I do need PO input on that.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/740/
Test FAILed.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/741/
Test PASSed.

@akx
Copy link
Contributor Author

akx commented Jun 25, 2015

As discussed in meatspace, I added a 24 hour grace period during which telemetry payloads will never be sent.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/742/
Test PASSed.


OPT_OUT_KWARGS = dict(module="telemetry", key="opt_out")
INSTALLATION_KEY_KWARGS = dict(module="telemetry", key="installation_key")
LAST_DATA_KWARGS = dict(module="telemetry", key="last_data")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you're using dict(foo=bar) rather than the normal {'foo': bar} construct here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see... It seems more logical now that I looked the code that uses them... These are OK.

@akx akx force-pushed the telemetry branch 3 times, most recently from c07e496 to e06abfd Compare June 29, 2015 11:16
@akx
Copy link
Contributor Author

akx commented Jun 29, 2015

retest this please

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/756/
Test PASSed.

<p>
The data contains an unique installation key,
as well as the hostname of the installation (as sent by the visiting browser; currently
<code>{{ request.get_host() }}</code>).
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, very nice improvement to the "HTTP client". Much better, IMHO. Thanks.

return data


def send_telemetry(request=None, max_age_hours=72, raise_on_error=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

max_age_hours should probably be a setting rather than argument here.

@suutari-ai
Copy link
Contributor

Btw, commit message title should not end with period.

@akx
Copy link
Contributor Author

akx commented Jun 30, 2015

Renamed to try_send_telemetry.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/760/
Test PASSed.

@anders-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
http://spartacus.aintra:8080//job/shoop-ng-pull-requests/761/
Test PASSed.

@suutari-ai suutari-ai merged commit 851add7 into shuup:master Jul 2, 2015
@akx akx deleted the telemetry branch July 2, 2015 09:27
tulimaki pushed a commit to tulimaki/shuup that referenced this pull request Jan 3, 2017
suutari-ai added a commit to andersinno/shuup that referenced this pull request Apr 4, 2018
…-bug

Allow longer region_codes in addresses
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.

4 participants