Closed Bug 941685 Opened 6 years ago Closed 6 years ago

Ping metrics server with snippets impression data

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

See bug 937373 for the spec we're going to use.
Attached patch patchSplinter Review
This applies on top of my patch from bug 937820.

While working on this, I realized we should remember to disable snippets on our test infrastucture whenever we turn them on by default, since we don't want metrics to be getting lots of pings from tinderbox devices.
Attachment #8336515 - Flags: review?(bnicholson)
Comment on attachment 8336515 [details] [diff] [review]
patch

Review of attachment 8336515 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/components/Snippets.js
@@ +179,5 @@
> + * @param snippetId unique id for snippet, sent from snippets server
> + * @param timestamp in ISO8601
> + */
> +function writeStat(snippetId, timestamp) {
> +  let path = OS.Path.join(OS.Constants.Path.profileDir, "snippets-stats.txt");

Might want to extract path into a constant since it's duplicated in several different methods.

@@ +192,5 @@
> +        yield file.close();
> +      }
> +    } catch (ex if ex instanceof OS.File.Error && ex.becauseNoSuchFile) {
> +      // If the file doesn't exist yet, create it.
> +      yield OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" });

Hmm...is this necessary? I would have expected a write via OS.File.open to create the file if it doesn't exist.

@@ +207,5 @@
> +  promise.then(array => sendStatsRequest(gDecoder.decode(array)), e => {
> +    if (e instanceof OS.File.Error && e.becauseNoSuchFile) {
> +      // If the file doesn't exist, there aren't any stats to send.
> +    } else {
> +      Cu.reportError("Error eading snippets stats: " + e);

Nit: eading
Attachment #8336515 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #2)

> @@ +192,5 @@
> > +        yield file.close();
> > +      }
> > +    } catch (ex if ex instanceof OS.File.Error && ex.becauseNoSuchFile) {
> > +      // If the file doesn't exist yet, create it.
> > +      yield OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" });
> 
> Hmm...is this necessary? I would have expected a write via OS.File.open to
> create the file if it doesn't exist.

I found that it was. OS.File.open seems pretty picky about the mode you can pass to open things:
https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#OS.File.open%28%29
(In reply to :Margaret Leibovic (away Nov 23 - Dec 1) from comment #3)
> I found that it was. OS.File.open seems pretty picky about the mode you can
> pass to open things:
> https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.
> File_for_the_main_thread#OS.File.open%28%29

Yeah, I ran into that page when looking over your patch, but it doesn't specify what happens when no mode is explicitly given. Using the optional "create" mode means that the file must not exist before writing; using the optional "existing" mode means the file must exist before writing. But you're using neither, so are you saying that the "existing" mode is used by default?
(In reply to Brian Nicholson (:bnicholson) from comment #4)
> (In reply to :Margaret Leibovic (away Nov 23 - Dec 1) from comment #3)
> > I found that it was. OS.File.open seems pretty picky about the mode you can
> > pass to open things:
> > https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.
> > File_for_the_main_thread#OS.File.open%28%29
> 
> Yeah, I ran into that page when looking over your patch, but it doesn't
> specify what happens when no mode is explicitly given. Using the optional
> "create" mode means that the file must not exist before writing; using the
> optional "existing" mode means the file must exist before writing. But
> you're using neither, so are you saying that the "existing" mode is used by
> default?

I am catching that error, so yes, I suppose that must be the default.

We can double-check with Yoric to make sure what we're doing here is the best way to approach this. Yoric, would you mind giving this patch a quick look to see if OS.File logic in there makes sense?
Flags: needinfo?(dteller)
Normally, using neither |create| nor |existing| should do the trick. If it's not the case, this is probably a bug.
Flags: needinfo?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #6)
> Normally, using neither |create| nor |existing| should do the trick. If it's
> not the case, this is probably a bug.

If I include neither, and the file hasn't been created yet, I get this error:
[JavaScript Error: "Error writing snippets stats: Unix error 2 during operation open (No such file or directory)" {file: "jar:jar:file:///data/app/org.mozilla.fennec_leibovic-1.apk!/assets/omni.ja!/components/Snippets.js" line: 220}]
I landed the patch as-is (although I pulled out the paths into lazy getters, since bnicholson pointed out that they were repeated):
https://hg.mozilla.org/integration/fx-team/rev/ef0cf280dd12

Yoric, I filed bug 945536 about the OS.File.open issue I ran into.
https://hg.mozilla.org/mozilla-central/rev/ef0cf280dd12
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.