Bug 1716956: Initial attempt to implement metric type "StringList"

[ChinYing-Li]

Pull Request checklist

• Quality: Make sure this PR builds and runs cleanly.
  • Inside the glean/ folder, run:
    • npm run test Runs all tests
    • npm run lint Runs all linters
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
• Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work
  • Documentation should be added to The Glean Book when making changes to Glean's user facing API
    • When changes to the Glean Book are necessary, link to the corresponding PR on the mozilla/glean repository
  • Documentation should be added to the Glean.js developer documentation when making internal code changes

Currently one unit test case and schema test fails. Opening a draft and is not ready for review.
EDIT: PR now ready for review; For StringMetricType, I have to declare add as async.

[brizental]

Hey @ChinYing-Li , just FYI I am on PTO this week and will come back to review this PR on Monday! Thank you for seding this in and sorry for the delayed response 👋

[brizental]

Great work here @ChinYing-Li ! This is definitely on the right path, nothing major is wrong and the apporach is correct. Left a few comments, let me know if you have any questions.

[brizental]

Currently one unit test case and schema test fails. Opening a draft and is not ready for review.

Heh, just saw this comment. Left a review anyways, hope it is helpful.

Regarding the schema test, it's possible it is failing because of the stringlist -> string_list change I requested in the review. The Glean schema is expecting the payload to contain string_list with the underline and anything different should cause a schema error.

[ChinYing-Li]

Without async means I have to write something like

truncatedValule = "";
 const truncatedValuePromise = truncateStringAtBoundaryWithError(this, value, MAX_STRING_LENGTH);
truncatedValuePromise.then((v) => {
  truncatedValue = v;
});

However the code above led to failure of unit test case attempt to add string to string list of maximum length records an error: the empty string "" was inserted instead.
Any suggestion is appreciated!

[brizental]

Without async means I have to write something like

truncatedValule = "";
 const truncatedValuePromise = truncateStringAtBoundaryWithError(this, value, MAX_STRING_LENGTH);
truncatedValuePromise.then((v) => {
  truncatedValue = v;
});

By wrapping the whole function body in a dispatcher.launch call, it will be inside an async function block so you can use await normally. You can see that pattern in action on the URL metric type set function (https://github.com/mozilla/glean.js/blob/main/glean/src/core/metrics/types/url.ts#L94-L113) or any other metric type.

The dispatcher orchestrates Glean's async tasks, so that they are run in the order the user called them. But that all happens in the background, without the user having to await on anything. That is a bit confusing, let me know if it's still unclear I am happy to try and clarify more :)

However the code above led to failure of unit test cas

They failure is probably happening because the code is not using the dispatcher, so at the time the assertion is called the action that should precede has not completed yet.

[ChinYing-Li]

@brizental I've addressed all the issues, and would be great if you can take a look. Thank you so much for the guidance!

[brizental]

This is looking great, I left a few nits, but nothing major. Once these are adressed this should be good to go.

Thanks for the work here @ChinYing-Li !

[brizental]

Hey @ChinYing-Li I see you addressed the review comments, but there are three still missing :) Would you mind addressing those and rebasing this PR so I can merge? Thank you so much :)

Also, the package-lock.json changes can be dropped. That has already ben fixed in a different PR.

[ChinYing-Li]

Hi @brizental , sorry for the delay! Please review and let me know if anything else needs to be fixed. Thank you.

[brizental]

This is looking perfect. Thanks so much for the great work here @ChinYing-Li .

Would you like to also take on the documentation updates over on the Glean book, for the string list metric type? That means adding the JavaScript examples to this page. If not, that is no problem, just let me know so I can file a follow up bug for that work.

Also, let me know if you want help to find a next bug, I am always happy to assist. Note that I will be on vacation next week. During that time, you can reach out to @Dexterp37 .

[ChinYing-Li]

Hi @brizental , Sure, I can update the Glean book!