Closed Bug 1344743 Opened 7 years ago Closed 7 years ago

Update TelemetryStorage.jsm to async function & await

Categories

(Toolkit :: Telemetry, enhancement, P4)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: gfritzsche, Assigned: subhdeepsaha, Mentored)

References

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(2 files, 1 obsolete file)

Bug 1344743 - Update TelemetryStorage.jsm to async function & await.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Error log of the unit tests
291.07 KB, text/plain
Details 
We have `async function` and `await` now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

We should be able to replace:
- `Task.async(function*(...))`, `Task.spawn(function*(...))` with a form of `async function ...`
- `yield` with `await` in the changed functions

https://dxr.mozilla.org/mozilla-central/search?q=path%3ATelemetryStorage.jsm+Task.&redirect=false

The only case that doesn't migrate trivially is any `DeferredTask` usage, we could just leave those for now.
Blocks: 1344744
No longer blocks: 1344744
Currently i am working on it and have made some changes, but i am unable to test these changes. Could you please help me?
(In reply to Subhdeep Saha from comment #2)
> Created attachment 8845383 [details]
> BUG:1344743 Changed some lines of code Task.async to async function..
> 
> Review commit: https://reviewboard.mozilla.org/r/118586/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/118586/

I have changed few functions to `asyn function()`. Could you please help me out how should do it for the dependent functions. They are failing the unit tests.
Assignee: nobody → subhdeepsaha
Attachment #8845383 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Could you please help in getting this bug fixed. Whenever i am calling a `x: async function()` by `await x()`. The unit tests fails
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8845383 [details]
Bug 1344743 - Update TelemetryStorage.jsm to async function & await.

https://reviewboard.mozilla.org/r/118586/#review120910

Please note that there are quite a few functions left in the file still using |Task.async|, you should change them too. Please also use the shorthand notation, otherwise eslint validation will fail.

Once you've changed all the function to use async/await, if there's nothing else using |Task|, please remove the related include from the top of the file.

In order to make sure that everything works, before requesting a new review, please run these tests:

- ./mach eslint toolkit/components/telemetry
- ./mach test toolkit/components/telemetry

::: commit-message-c40ca:1
(Diff revision 1)
> +BUG:1344743 Changed some lines of code Task.async to async function.. r=gfritzsche

Please reformat your commit message so that it follows che convention specified [here](https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions).

This should become: "Bug 1344743 - Update TelemetryStorage.jsm to async function & await. r?dexter"

::: toolkit/components/telemetry/TelemetryStorage.jsm:422
(Diff revision 1)
>     * Loads a ping file.
>     * @param {String} aFilePath The path of the ping file.
>     * @return {Promise<Object>} A promise resolved with the ping content or rejected if the
>     *                           ping contains invalid data.
>     */
> -  loadPingFile: Task.async(function* (aFilePath) {
> +  // loadPingFile: Task.async(function* (aFilePath) {

Please drop the commented code here.

::: toolkit/components/telemetry/TelemetryStorage.jsm:426
(Diff revision 1)
>     */
> -  loadPingFile: Task.async(function* (aFilePath) {
> +  // loadPingFile: Task.async(function* (aFilePath) {
> +    // return TelemetryStorageImpl.loadPingFile(aFilePath);
> +  // }),
> +
> +  loadPingFile: async function(aFilePath) {

Please use the [shorthand notation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods) here, otherwise eslint will fail.

This should become:

    async loadPingFile(aFilePath) {

::: toolkit/components/telemetry/TelemetryStorage.jsm:612
(Diff revision 1)
>    /**
>     * Shutdown & block on any outstanding async activity in this module.
>     *
>     * @return {Promise} Promise that is resolved when shutdown is complete.
>     */
> -  shutdown: Task.async(function*() {
> +  shutdown: async function() {

Please use the [shorthand notation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods) here.

::: toolkit/components/telemetry/TelemetryStorage.jsm:664
(Diff revision 1)
>      promise.then((r) => { this._activelyArchiving.delete(promise); },
>                   (e) => { this._activelyArchiving.delete(promise); });
>      return promise;
>    },
>  
> -  _saveArchivedPingTask: Task.async(function*(ping) {
> +  _saveArchivedPingTask: async function(ping) {

Please use the [shorthand notation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods) here.

::: toolkit/components/telemetry/TelemetryStorage.jsm:741
(Diff revision 1)
>     */
>    saveSessionData(sessionData) {
>      return this._stateSaveSerializer.enqueueTask(() => this._saveSessionData(sessionData));
>    },
>  
> -  _saveSessionData: Task.async(function* (sessionData) {
> +  _saveSessionData: async function(sessionData) {

Please use the [shorthand notation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods) here.

::: toolkit/components/telemetry/TelemetryStorage.jsm:763
(Diff revision 1)
>     */
>    loadSessionData() {
>      return this._stateSaveSerializer.enqueueTask(() => this._loadSessionData());
>    },
>  
> -  _loadSessionData: Task.async(function* () {
> +  _loadSessionData: async function () {

Please use the [shorthand notation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods) here.

::: toolkit/components/telemetry/TelemetryStorage.jsm:795
(Diff revision 1)
>     * @param {string} id The pings id.
>     * @param {number} timestampCreated The pings creation timestamp.
>     * @param {string} type The pings type.
>     * @return {promise<object>} Promise that is resolved when the pings is removed.
>     */
> -  _removeArchivedPing: Task.async(function*(id, timestampCreated, type) {
> +  _removeArchivedPing: async function(id, timestampCreated, type) {

Please use the [shorthand notation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods) here.
Attachment #8845383 - Flags: review?(alessio.placitelli)
(In reply to Subhdeep Saha from comment #4)
> Could you please help in getting this bug fixed. Whenever i am calling a `x:
> async function()` by `await x()`. The unit tests fails

I forgot to mention that if you're still getting failure after the changes requested above, please post the relevant portion of the log describing the failure!
Flags: needinfo?(alessio.placitelli)
Flags: needinfo?(alessio.placitelli)

    
    

    
  
Comment on attachment 8845383 [details]
Bug 1344743 - Update TelemetryStorage.jsm to async function & await.

https://reviewboard.mozilla.org/r/118586/#review121004

::: commit-message-c40ca:1
(Diff revision 2)
> +Bug 1344743 - Update TelemetryStorage.jsm to async function & await. r=Dexter

The last part of the commit message should be "r?dexter" rather than "r=dexter". As the "=" is used to indicate that a r+ was given :)

::: toolkit/components/telemetry/TelemetryStorage.jsm:109
(Diff revision 2)
>   */
>  var Policy = {
>    now: () => new Date(),
>    getArchiveQuota: () => ARCHIVE_QUOTA_BYTES,
>    getPendingPingsQuota: () => (AppConstants.platform in ["android", "gonk"])
> -                                ? PENDING_PINGS_QUOTA_BYTES_MOBILE
> +    ? PENDING_PINGS_QUOTA_BYTES_MOBILE

Please drop the changes to this line and the one below.

::: toolkit/components/telemetry/TelemetryStorage.jsm:291
(Diff revision 2)
>     *
>     * @return {Promise<sequence>} Resolved with the ping list.
>     */
>    loadPendingPingList() {
>      return TelemetryStorageImpl.loadPendingPingList();
> -   },
> +  },

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:305
(Diff revision 2)
>     *
>     * @return {sequence} The current pending ping list.
>     */
>    getPendingPingList() {
>      return TelemetryStorageImpl.getPendingPingList();
> -   },
> +  },

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:490
(Diff revision 2)
>     * @param {Function} aFunction The task function to enqueue. It must return a promise.
>     * @return {Promise} A promise resolved when the enqueued task completes.
>     */
>    enqueueTask(aFunction) {
>      let promise = new Promise((resolve, reject) =>
> -      this._queuedOperations.push([aFunction, resolve, reject]));
> +                              this._queuedOperations.push([aFunction, resolve, reject]));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:547
(Diff revision 2)
>      promise.then(result => {
> -        this._queuedInProgress = false;
> +      this._queuedInProgress = false;
> -        resolve(result);
> +      resolve(result);
> -        this._popAndPerformQueuedOperation();
> +      this._popAndPerformQueuedOperation();
> -      },
> +    },
> -      error => {
> +                 error => {

Please drop these changes as they are not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:659
(Diff revision 2)
>      promise.then((r) => { this._activelyArchiving.delete(promise); },
>                   (e) => { this._activelyArchiving.delete(promise); });
>      return promise;
>    },
>  
> -  _saveArchivedPingTask: async function(ping) {
> +  async _SaveArchivedPingTask(ping) {

Please don't change the name of the function. This should still be "\_saveArchivedPingTask", with a lowercase s.

::: toolkit/components/telemetry/TelemetryStorage.jsm:703
(Diff revision 2)
>      const path = getArchivedPingPath(id, new Date(data.timestampCreated), data.type);
>      const pathCompressed = path + "lz4";
>  
>      // Purge pings which are too big.
>      let checkSize = function*(path) {
>        const fileSize = (yield OS.File.stat(path)).size;

The yield here should be await.

::: toolkit/components/telemetry/TelemetryStorage.jsm:706
(Diff revision 2)
>      // Purge pings which are too big.
>      let checkSize = function*(path) {
>        const fileSize = (yield OS.File.stat(path)).size;
>        if (fileSize > PING_FILE_MAXIMUM_SIZE_BYTES) {
>          Telemetry.getHistogramById("TELEMETRY_DISCARDED_ARCHIVED_PINGS_SIZE_MB")
> -                 .add(Math.floor(fileSize / 1024 / 1024));
> +          .add(Math.floor(fileSize / 1024 / 1024));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:708
(Diff revision 2)
>        const fileSize = (yield OS.File.stat(path)).size;
>        if (fileSize > PING_FILE_MAXIMUM_SIZE_BYTES) {
>          Telemetry.getHistogramById("TELEMETRY_DISCARDED_ARCHIVED_PINGS_SIZE_MB")
> -                 .add(Math.floor(fileSize / 1024 / 1024));
> +          .add(Math.floor(fileSize / 1024 / 1024));
>          Telemetry.getHistogramById("TELEMETRY_PING_SIZE_EXCEEDED_ARCHIVED").add();
>          yield OS.File.remove(path, {ignoreAbsent: true});

You didn't change the yield here. This should be await.

::: toolkit/components/telemetry/TelemetryStorage.jsm:716
(Diff revision 2)
>      };
>  
>      try {
>        // Try to load a compressed version of the archived ping first.
>        this._log.trace("loadArchivedPing - loading ping from: " + pathCompressed);
>        yield* checkSize(pathCompressed);

The yield here should be await.

::: toolkit/components/telemetry/TelemetryStorage.jsm:724
(Diff revision 2)
>        if (!ex.becauseNoSuchFile) {
>          throw ex;
>        }
>        // If that fails, look for the uncompressed version.
>        this._log.trace("loadArchivedPing - compressed ping not found, loading: " + path);
>        yield* checkSize(path);

The yield here should be await.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1098
(Diff revision 2)
>        }
>      }
>  
>      // Save the time it takes to check if the pending pings are over-quota.
>      Telemetry.getHistogramById("TELEMETRY_PENDING_CHECKING_OVER_QUOTA_MS")
> -             .add(Math.round(Policy.now().getTime() - startTimeStamp));
> +      .add(Math.round(Policy.now().getTime() - startTimeStamp));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1135
(Diff revision 2)
>      const endTimeStamp = Policy.now().getTime();
>      // We don't know the size of the pending pings directory if we are above the quota,
>      // since we stop scanning once we reach the quota. We use a special value to show
>      // this condition.
>      recordHistograms(PENDING_PINGS_SIZE_PROBE_SPECIAL_VALUE, pingsToPurge.length,
> -                 Math.ceil(endTimeStamp - startTimeStamp));
> +                     Math.ceil(endTimeStamp - startTimeStamp));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1156
(Diff revision 2)
>     * This will scan the archive directory and grab basic data about the existing
>     * pings out of their filename.
>     *
>     * @return {promise<sequence<object>>}
>     */
> -  loadArchivedPingList: Task.async(function*() {
> +  async loadArchicedPingList() {

Please don't change the name of the function.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1185
(Diff revision 2)
> -  _scanArchive: Task.async(function*() {
> +  async _scanArchive() {
>      this._log.trace("_scanArchive");
>  
>      let submitProbes = (pingCount, dirCount) => {
>        Telemetry.getHistogramById("TELEMETRY_ARCHIVE_SCAN_PING_COUNT")
> -               .add(pingCount);
> +        .add(pingCount);

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1187
(Diff revision 2)
>  
>      let submitProbes = (pingCount, dirCount) => {
>        Telemetry.getHistogramById("TELEMETRY_ARCHIVE_SCAN_PING_COUNT")
> -               .add(pingCount);
> +        .add(pingCount);
>        Telemetry.getHistogramById("TELEMETRY_ARCHIVE_DIRECTORIES_COUNT")
> -               .add(dirCount);
> +        .add(dirCount);

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1225
(Diff revision 2)
>            if (!overwrite) {
>              continue;
>            }
>  
> -          yield this._removeArchivedPing(data.id, data.timestampCreated, data.type)
> +          await this._removeArchivedPing(data.id, data.timestampCreated, data.type)
> -                    .catch((e) => this._log.warn("_scanArchive - failed to remove ping", e));
> +            .catch((e) => this._log.warn("_scanArchive - failed to remove ping", e));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1342
(Diff revision 2)
>  
>      // Purge pings which are too big.
>      if (fileSize > PING_FILE_MAXIMUM_SIZE_BYTES) {
> -      yield this.removePendingPing(id);
> +      await this.removePendingPing(id);
>        Telemetry.getHistogramById("TELEMETRY_DISCARDED_PENDING_PINGS_SIZE_MB")
> -               .add(Math.floor(fileSize / 1024 / 1024));
> +        .add(Math.floor(fileSize / 1024 / 1024));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1378
(Diff revision 2)
>  
>      this._log.trace("removePendingPing - deleting ping with id: " + id +
>                      ", path: " + info.path);
>      this._pendingPings.delete(id);
>      return OS.File.remove(info.path).catch((ex) =>
> -      this._log.error("removePendingPing - failed to remove ping", ex));
> +                                           this._log.error("removePendingPing - failed to remove ping", ex));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1519
(Diff revision 2)
> -            yield OS.File.remove(file.path);
> +            await OS.File.remove(file.path);
>            } catch (ex) {
>              this._log.error("_scanPendingPings - failed to remove file " + file.path, ex);
>            } finally {
>              Telemetry.getHistogramById("TELEMETRY_DISCARDED_PENDING_PINGS_SIZE_MB")
> -                     .add(Math.floor(info.size / 1024 / 1024));
> +              .add(Math.floor(info.size / 1024 / 1024));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1661
(Diff revision 2)
>        id: uuid,
>        type,
>      };
>    },
>  
> -  saveAbortedSessionPing: Task.async(function*(ping) {
> +  async function saveAbortedSessionPing(ping) {

You should drop "function".

::: toolkit/components/telemetry/TelemetryStorage.jsm:1663
(Diff revision 2)
>      };
>    },
>  
> -  saveAbortedSessionPing: Task.async(function*(ping) {
> +  async function saveAbortedSessionPing(ping) {
>      this._log.trace("saveAbortedSessionPing - ping path: " + gAbortedSessionFilePath);
>      yield OS.File.makeDir(gDataReportingDir, { ignoreExisting: true });

The yield here should be await.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1666
(Diff revision 2)
> -  saveAbortedSessionPing: Task.async(function*(ping) {
> +  async function saveAbortedSessionPing(ping) {
>      this._log.trace("saveAbortedSessionPing - ping path: " + gAbortedSessionFilePath);
>      yield OS.File.makeDir(gDataReportingDir, { ignoreExisting: true });
>  
>      return this._abortedSessionSerializer.enqueueTask(() =>
> -      this.savePingToFile(ping, gAbortedSessionFilePath, true));
> +                                                      this.savePingToFile(ping, gAbortedSessionFilePath, true));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1695
(Diff revision 2)
>            this._log.trace("removeAbortedSessionPing - no such file");
>          } else {
>            this._log.error("removeAbortedSessionPing - error removing ping", ex)
>          }
>        }
> -    }.bind(this)));
> +    }.bind(this))();

Why are you adding the trailing "()"?

::: toolkit/components/telemetry/TelemetryStorage.jsm:1708
(Diff revision 2)
> -  saveDeletionPing: Task.async(function*(ping) {
> +  async saveDeletionPing(ping) {
>      this._log.trace("saveDeletionPing - ping path: " + gDeletionPingFilePath);
> -    yield OS.File.makeDir(gDataReportingDir, { ignoreExisting: true });
> +    await OS.File.makeDir(gDataReportingDir, { ignoreExisting: true });
>  
>      let p = this._deletionPingSerializer.enqueueTask(() =>
> -      this.savePingToFile(ping, gDeletionPingFilePath, true));
> +                                                     this.savePingToFile(ping, gDeletionPingFilePath, true));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1729
(Diff revision 2)
>            this._log.trace("removeDeletionPing - no such file");
>          } else {
>            this._log.error("removeDeletionPing - error removing ping", ex)
>          }
>        }
> -    }.bind(this)));
> +    }.bind(this))();

Why are you adding the trailing "()"?

::: toolkit/components/telemetry/TelemetryStorage.jsm:1767
(Diff revision 2)
>      ];
>  
>      // FHR could have used either the default DB file name or a custom one
>      // through this preference.
>      const FHR_DB_CUSTOM_FILENAME =
> -      Preferences.get("datareporting.healthreport.dbName", undefined);
> +          Preferences.get("datareporting.healthreport.dbName", undefined);

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1777
(Diff revision 2)
>          OS.Path.join(OS.Constants.Path.profileDir, FHR_DB_CUSTOM_FILENAME + "-shm"));
>      }
>  
>      for (let f of FILES_TO_REMOVE) {
> -      yield OS.File.remove(f, {ignoreAbsent: true})
> +      await OS.File.remove(f, {ignoreAbsent: true})
> -                   .catch(e => this._log.error("removeFHRDatabase - failed to remove " + f, e));
> +        .catch(e => this._log.error("removeFHRDatabase - failed to remove " + f, e));

Please drop this change as it's not related to this bug.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1814
(Diff revision 2)
>  function getArchivedPingPath(aPingId, aDate, aType) {
>    // Get the ping creation date and generate the archive directory to hold it. Note
>    // that getMonth returns a 0-based month, so we need to add an offset.
>    let month = new String(aDate.getMonth() + 1);
>    let archivedPingDir = OS.Path.join(gPingsArchivePath,
> -    aDate.getFullYear() + "-" + month.padStart(2, "0"));
> +                                     aDate.getFullYear() + "-" + month.padStart(2, "0"));

Please drop this change as it's not related to this bug.

        Attachment #8845383 -
        Flags: review?(alessio.placitelli)

    
    

    
  
(In reply to Subhdeep Saha from comment #8)
> Created attachment 8845814 [details]
> Error log of the unit tests

If you look at the log, you can see the following line:

>  0:15.11 LOG: Thread-55 ERROR SyntaxError: yield is a reserved identifier at resource://gre/modules/TelemetryStorage.jsm:716

It is telling you that you're using "yield" when you're not allowed to, at TelemetryStorage.jsm  line 716.
That's because, as stated in comment 0:

> - (change) `yield` with `await` in the changed functions

Please note that you don't need to flag me for needinfo if you already requested a review. Also, make sure to request a review only when your code is working and passing the tests.
Flags: needinfo?(alessio.placitelli)

    
    

    
  
i have changed all the yield to async but still the unit tests are failing. 
i cannot understand the following error log:
` [JavaScript Error: "1489255787761  Toolkit.Telemetry ERROR TelemetrySend::setup - _checkPendingPings rejected: SyntaxError: missing ) in parenthetical (resource://gre/modules/TelemetryStorage.jsm:703:30) `

should i send for a review?

    
    

    
  
@Subhadep, Hi you need to change yield to await and Task.async to async <function_name> () {}

    
    

    
  
(In reply to Deepjyoti Mondal from comment #12)
> @Subhadep, Hi you need to change yield to await and Task.async to async
> <function_name> () {}

ya i have done that. still the tests are failing.

    
    

    
  
(In reply to Subhdeep Saha from comment #11)
> i have changed all the yield to async but still the unit tests are failing. 
> i cannot understand the following error log:
> ` [JavaScript Error: "1489255787761  Toolkit.Telemetry ERROR
> TelemetrySend::setup - _checkPendingPings rejected: SyntaxError: missing )
> in parenthetical (resource://gre/modules/TelemetryStorage.jsm:703:30) `
> 
> should i send for a review?

Let's take a look at the error message:

> _checkPendingPings rejected: SyntaxError: missing )
> in parenthetical (resource://gre/modules/TelemetryStorage.jsm:703:30) `

The first line is telling you that there's probably a missing parenthesis.
The second line gives you an hint about where to look: it's telling you to look at the "TelemetryStorage.jsm" file, line 703, column 30.

Have a look at the suggested locations and see if you can spot the error. If you can, and are able to fix it, good! If you need further help, feel free to push your updated code so that I can take a look ;)

    
    

    
      
  

    
    

    
  
The patch you sent on MozReview isn't addressing any of the issues that were raised in the previous review pass. Moreover, it doesn't apply cleanly to the latest mozilla-central as it seems to be based off the code from a previous work in progress patch.

> -  // loadPingFile: Task.async(function* (aFilePath) {
> -    // return TelemetryStorageImpl.loadPingFile(aFilePath);
> -  // }),
> -
> -  loadPingFile: async function(aFilePath) {
> +  async loadPingFile(aFilePath) {

Please:

- Rebase your commit so that it's based off the latest mozilla-central
- Address all the feedback in the previous pass
- Make sure the tests succeed. If they don't, try to figure out what's wrong by looking at the logs.
Flags: needinfo?(subhdeepsaha)
Flags: needinfo?(subhdeepsaha)

        Attachment #8845383 -
        Flags: review?(alessio.placitelli)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
please check the last review. All the unit tests are passing :)

    
    

    
  
Comment on attachment 8845383 [details]
Bug 1344743 - Update TelemetryStorage.jsm to async function & await.

Nice that this is actually passing tests right now. Unfortunately, we can't still land it, as the committed patch can't be directly applied to mozilla-central.

> -  // loadPingFile: Task.async(function* (aFilePath) {
> -    // return TelemetryStorageImpl.loadPingFile(aFilePath);
> -  // }),
> -
> -  loadPingFile: async function(aFilePath) {
> +  async loadPingFile(aFilePath) {

As hinted in comment 16, you probably have one of the following conditions:

- Two commits
- An applied patch file and a commit

Basically, somewhere before the commit you're requesting a review for you have some patch/other commit that introduced this line(s):

> // loadPingFile: Task.async(function* (aFilePath) {

That code isn't in mozilla-central. Please rebase your patch on the latest mozilla-central and push an updated patch.

        Attachment #8845383 -
        Flags: review?(alessio.placitelli)

    
    

    
  
Hi Alessio,
 Could you please help me doing rebase, I tried it yesterday doing `hg rebase` and then i had sent this patch for review, i think that didn't work.

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
Comment on attachment 8848054 [details]
BUG:1344743 Changed some lines of code Task.async to async function..

I think it would be better if I kept reviewing this, as I have better context :)

        Attachment #8848054 -
        Flags: review?(gfritzsche)

    
    

    
  
(In reply to Subhdeep Saha from comment #21)
> Hi Alessio,
>  Could you please help me doing rebase, I tried it yesterday doing `hg
> rebase` and then i had sent this patch for review, i think that didn't work.

Yeah, I think so :) It looks like you have two different commits at the tip of your repository: the first one (oldest) is the one with your earliest changes, the second one (the newest) is the one I recently reviewed.

I think you forgot to "amend" the commit the first time you committed, and now you have them both. I think you might have two options now:

1) Squash the two commits into one and push the squashed commit to review.
2) Remove both commits and redo the changes from scratch, then push this single commit for review.

I'm not sure if option (1) creates any problem with MozReview, you might try and we can see. You could probably try in #introduction, as maybe somebody else has better ideas!

I'm clearing both review requests: feel free to flag me again once you get a working patch.

        Attachment #8845383 -
        Flags: review?(alessio.placitelli)

    
    

    
  
(In reply to Alessio Placitelli [:Dexter] from comment #25)
> (In reply to Subhdeep Saha from comment #21)
> > Hi Alessio,
> >  Could you please help me doing rebase, I tried it yesterday doing `hg
> > rebase` and then i had sent this patch for review, i think that didn't work.
> 
> Yeah, I think so :) It looks like you have two different commits at the tip
> of your repository: the first one (oldest) is the one with your earliest
> changes, the second one (the newest) is the one I recently reviewed.
> 
> I think you forgot to "amend" the commit the first time you committed, and
> now you have them both. I think you might have two options now:
> 
> 1) Squash the two commits into one and push the squashed commit to review.

Mh, thinking again, there's even a better way to do that, as suggested by the docs. See https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#changing-code-after-reviews

Simply use histedit to squash the two commits.
Flags: needinfo?(subhdeepsaha)

    
    

    
      
  

    
  

        Attachment #8848054 -
        Attachment is obsolete: true

    
    

    
  
Hi Alessio ,
 Please check the latest patch, I think the commit messages have been handled carefully. :)

    
    

    
  
Comment on attachment 8845383 [details]
Bug 1344743 - Update TelemetryStorage.jsm to async function & await.

https://reviewboard.mozilla.org/r/118586/#review124014

This is functionally correct now, thank you! You just need to drop the unrelated whitespace/indentation changes that you made and we should be good to go.

Please remember to push one single commit for review with the updated stuff, as you did this time!

One last thing: before pushing the updates, please build and make sure that "./mach test toolkit/components/telemetry" runs with no error. Same thing for "./mach eslint toolkit/components/telemetry".

::: toolkit/components/telemetry/TelemetryStorage.jsm:896
(Diff revision 7)
>  
>      const endTimeStamp = Policy.now().getTime();
>  
>      // Save the time it takes to evict old directories and the eviction count.
>      Telemetry.getHistogramById("TELEMETRY_ARCHIVE_EVICTED_OLD_DIRS")
> -             .add(evictedDirsCount);
> +      .add(evictedDirsCount);

Please drop the change to this line and keep the indentation.

::: toolkit/components/telemetry/TelemetryStorage.jsm:898
(Diff revision 7)
>  
>      // Save the time it takes to evict old directories and the eviction count.
>      Telemetry.getHistogramById("TELEMETRY_ARCHIVE_EVICTED_OLD_DIRS")
> -             .add(evictedDirsCount);
> +      .add(evictedDirsCount);
>      Telemetry.getHistogramById("TELEMETRY_ARCHIVE_EVICTING_DIRS_MS")
> -             .add(Math.ceil(endTimeStamp - startTimeStamp));
> +      .add(Math.ceil(endTimeStamp - startTimeStamp));

Please drop the change to this line and keep the indentation.

::: toolkit/components/telemetry/TelemetryStorage.jsm:900
(Diff revision 7)
>      Telemetry.getHistogramById("TELEMETRY_ARCHIVE_EVICTED_OLD_DIRS")
> -             .add(evictedDirsCount);
> +      .add(evictedDirsCount);
>      Telemetry.getHistogramById("TELEMETRY_ARCHIVE_EVICTING_DIRS_MS")
> -             .add(Math.ceil(endTimeStamp - startTimeStamp));
> +      .add(Math.ceil(endTimeStamp - startTimeStamp));
>      Telemetry.getHistogramById("TELEMETRY_ARCHIVE_OLDEST_DIRECTORY_AGE")
> -             .add(maxDirAgeInMonths);
> +      .add(maxDirAgeInMonths);

Please drop the change to this line and keep the indentation.

::: toolkit/components/telemetry/TelemetryStorage.jsm:938
(Diff revision 7)
>  
>        let ping = pingList[i];
>  
>        // Get the size for this ping.
>        const fileSize =
> -        yield getArchivedPingSize(ping.id, new Date(ping.timestampCreated), ping.type);
> +            await getArchivedPingSize(ping.id, new Date(ping.timestampCreated), ping.type);

Please use 2 whitespaces for indenting this line.

::: toolkit/components/telemetry/TelemetryStorage.jsm:950
(Diff revision 7)
>        if (fileSize > PING_FILE_MAXIMUM_SIZE_BYTES) {
>          this._log.error("_enforceArchiveQuota - removing file exceeding size limit, size: " + fileSize);
>          // We just remove the ping from the disk, we don't bother removing it from pingList
>          // since it won't contribute to the quota.
> -        yield this._removeArchivedPing(ping.id, ping.timestampCreated, ping.type)
> +        await this._removeArchivedPing(ping.id, ping.timestampCreated, ping.type)
> -                  .catch(e => this._log.error("_enforceArchiveQuota - failed to remove archived ping" + ping.id));
> +          .catch(e => this._log.error("_enforceArchiveQuota - failed to remove archived ping" + ping.id));

Please drop the change to this line and keep the indentation.

::: toolkit/components/telemetry/TelemetryStorage.jsm:952
(Diff revision 7)
>          // We just remove the ping from the disk, we don't bother removing it from pingList
>          // since it won't contribute to the quota.
> -        yield this._removeArchivedPing(ping.id, ping.timestampCreated, ping.type)
> +        await this._removeArchivedPing(ping.id, ping.timestampCreated, ping.type)
> -                  .catch(e => this._log.error("_enforceArchiveQuota - failed to remove archived ping" + ping.id));
> +          .catch(e => this._log.error("_enforceArchiveQuota - failed to remove archived ping" + ping.id));
>          Telemetry.getHistogramById("TELEMETRY_DISCARDED_ARCHIVED_PINGS_SIZE_MB")
> -                 .add(Math.floor(fileSize / 1024 / 1024));
> +          .add(Math.floor(fileSize / 1024 / 1024));

Please drop the change to this line and keep the indentation.

::: toolkit/components/telemetry/TelemetryStorage.jsm:971
(Diff revision 7)
>        }
>      }
>  
>      // Save the time it takes to check if the archive is over-quota.
>      Telemetry.getHistogramById("TELEMETRY_ARCHIVE_CHECKING_OVER_QUOTA_MS")
> -             .add(Math.round(Policy.now().getTime() - startTimeStamp));
> +      .add(Math.round(Policy.now().getTime() - startTimeStamp));

Please drop the change to this line and keep the indentation.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1136
(Diff revision 7)
>      const endTimeStamp = Policy.now().getTime();
>      // We don't know the size of the pending pings directory if we are above the quota,
>      // since we stop scanning once we reach the quota. We use a special value to show
>      // this condition.
>      recordHistograms(PENDING_PINGS_SIZE_PROBE_SPECIAL_VALUE, pingsToPurge.length,
> -                 Math.ceil(endTimeStamp - startTimeStamp));
> +                  Math.ceil(endTimeStamp - startTimeStamp));

Please drop the change to this line.

        Attachment #8845383 -
        Flags: review?(alessio.placitelli)

    
    

    
      
  

    
    

    
  
Comment on attachment 8845383 [details]
Bug 1344743 - Update TelemetryStorage.jsm to async function & await.

https://reviewboard.mozilla.org/r/118586/#review124394

This looks good now, thank you for your persistence!

Let me push this to Try to make sure nothing breaks due to these changes, as we had some unexpected fallout with other patches dealing with the async/await changes!

        Attachment #8845383 -
        Flags: review?(alessio.placitelli) → review+

    
    

    
  
Comment on attachment 8845383 [details]
Bug 1344743 - Update TelemetryStorage.jsm to async function & await.

https://reviewboard.mozilla.org/r/118586/#review124846

    
    

    
  
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e57f79e5212f
Update TelemetryStorage.jsm to async function & await. r=Dexter

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e57f79e5212f
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(subhdeepsaha)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: