Skip to content
This repository was archived by the owner on Jan 17, 2023. It is now read-only.

Add /api/metrics/testpilottest POST resource#952

Closed
lmorchard wants to merge 1 commit into
mozilla:masterfrom
lmorchard:433-metrics-ping-api
Closed

Add /api/metrics/testpilottest POST resource#952
lmorchard wants to merge 1 commit into
mozilla:masterfrom
lmorchard:433-metrics-ping-api

Conversation

@lmorchard
Copy link
Copy Markdown
Contributor

@lmorchard lmorchard commented Jun 6, 2016

Add /api/metrics/testpilottest POST resource

  • /api/metrics/testpilottest accepts POST'd JSON body which gets
    included in the Fields property of a mozlog log event
  • Add django-cors-headers dependency to allow CORS requests to the API
  • Tiny WebExtension example in docs/examples/webextension to show how to
    POST to the metrics ping resource
  • Tests

Issue #433

@lmorchard
Copy link
Copy Markdown
Contributor Author

Rebased against master. Made further tweaks to ensure a WebExtension can POST to the metrics API.

console.log('problem sending metrics ping', e);
}).then(resp => {
console.log('metrics ping success', resp);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How does .catch().then() work?
I can't remember if that means if you did catch an error it'd fall through and, you'd get both messages:

  • "problems sending metrics ping" AND
  • "metrics ping success"

Or should we reverse the order so it's .then().catch()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it bails on the catch, but I swapped the order anyway

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Turns out the catch() in the middle lets you handle the error and continue, so it'd have done both .catch() and .then():

function promiseTest(data = new Error("uh oh!")) {
  return new Promise((resolve, reject) => {
    if (data instanceof Error) {
      throw data;
    }
    resolve(data);
  });
}

promiseTest( /* error */ )
  .then((data) => {
    console.log(`first then(): ${data}`);
    return data;
  }).catch((err) => {
    console.error(`An error? ${err}`);
    return err;
  }).then((data) => {
    console.info(`after catch()? ${data}`);
  });
$ node promise.test.js

An error? Error: uh oh!
after catch()? Error: uh oh!

- /api/metrics/testpilottest accepts POST'd JSON body which gets
  included in the Fields property of a [mozlog log event][mozlog]

- Add django-cors-headers dependency to allow CORS requests to the API

- Tiny WebExtension example in docs/examples/webextension to show how to
  POST to the metrics ping resource

- Tests

[mozlog]: https://github.com/mozilla-services/Dockerflow/blob/master/docs/mozlog.md

Issue mozilla#433
@lmorchard lmorchard force-pushed the 433-metrics-ping-api branch from 6c217af to 6576b37 Compare June 7, 2016 21:10
@ghost
Copy link
Copy Markdown

ghost commented Jun 7, 2016

maybe a section at the bottom of readme-metrics.md about how to use this (format of the JSON if nothing else?)

@lmorchard
Copy link
Copy Markdown
Contributor Author

lmorchard commented Jun 7, 2016

I'm totally fuzzy on what the format of the JSON should be - that depends entirely on what gets done in the processing pipeline with the Fields section of the log events, and what the experiment wants to measure

@lmorchard
Copy link
Copy Markdown
Contributor Author

lmorchard commented Jun 7, 2016

Probably the closest thing is this section of README-METRICS that talks about what happens with request.summary server log events.

I'm thinking we'll probably have to see what a WebExtension experiment wants to measure and tweak the RedShift schema for a new testpilottest server event and the associated Fields properties. Maybe some will be similar to that generic request.summary schema, but needs some discussion

@ghost
Copy link
Copy Markdown

ghost commented Jun 8, 2016

Hmm, yeah. I thought we had an undefined payload{} section like we have for the client side...

I have a draft of metrics for firefoxnomore404s at internetarchive/wayback-machine-firefox#10 . If that draft gets approved, this is only recording a very small set of data. We could just overload the context field here to log the action of the user. (this makes sense if you look at the JSON at the bottom of my doc).

It's hacky, but if this is only temporary until webextensions can do their own logging.....

...talk me out of doing this the hacky way... 🔊

@permission_classes([])
def log_ping(request, logger_name):
"""Accept and log metrics pings"""
whitelist = getattr(settings, 'LOG_PING_WHITELIST', DEFAULT_LOG_PING_WHITELIST)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So we will need to add logger names for each experiment that uses this? I.e., for 404s and my "blok" add-on/experiment, we would need to have:
settings.py:

LOG_PING_WHITELIST = [
    'blok',
    'nomore404s'
]

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, this should be like the general testpilottest ping in Telemetry. One log event type (and URL) for all experiments. You should include the experiment's add-on ID in the JSON payload. Don't really have a schema for that payload, but i figured it could look like the telemetry ping.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FWIW, the whitelist is kind of a leftover from when we thought we'd be doing more of our own metrics with events like newuser and newinstall

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we update the docs/examples/webextension/background.js:fetch to include a more realistic example of the POST body with add-on ID?

  body: JSON.stringify({
    service: "blok@mozilla",
    msg: { domain: "economist.com", blockedTrackers: ["googleadservices.com", "facebook.net", "doubleclick.net"], reason: "broken"}
  })

Or whatever the add-ons need to POST that isn't auto-filled by the server?

@lmorchard
Copy link
Copy Markdown
Contributor Author

Going to close this, for now. Hopefully we can get #1008 into usable shape and get full Telemetry pings for webextensions

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants