Update javascript server outputter to support mixed-style collection

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - BREAKING CHANGE: Expose the optional `enabled` property on pings, defaulting to `enabled: true` ([#681](https://github.com/mozilla/glean_parser/pull/681))
 - BREAKING CHANGE: Support metadata field `ping_schedule` for pings ([bug 1804711](https://bugzilla.mozilla.org/show_bug.cgi?id=1804711))
+- Add support for event metric type in server JavaScript outputter ([DENG-2407](https://mozilla-hub.atlassian.net/browse/DENG-2407))
 
 ## 13.0.1
 

glean_parser/javascript_server.py

@@ -42,9 +42,12 @@ from . import util
 SUPPORTED_METRIC_TYPES = ["string", "event"]
 
 
-def event_class_name(ping_name: str, event_metric_exists: bool) -> str:
+def event_class_name(
+    ping_name: str, metrics_by_type: Dict[str, List[metrics.Metric]]
+) -> str:
     # For compatibility with FxA codebase we don't want to add "Logger" suffix
     # when custom pings without event metrics are used.
+    event_metric_exists = "event" in metrics_by_type
     suffix = "Logger" if event_metric_exists else ""
     return util.Camelize(ping_name) + "ServerEvent" + suffix
 
@@ -61,10 +64,13 @@ def generate_js_metric_type(metric: metrics.Metric) -> str:
     return metric.type
 
 
-def generate_ping_factory_method(ping: str, event_metric_exists: bool) -> str:
+def generate_ping_factory_method(
+    ping: str, metrics_by_type: Dict[str, List[metrics.Metric]]
+) -> str:
     # `ServerEventLogger` better describes role of the class that this factory
     # method generates, but for compatibility with existing FxA codebase
     # we use `Event` suffix if no event metrics are defined.
+    event_metric_exists = "event" in metrics_by_type
     suffix = "ServerEventLogger" if event_metric_exists else "Event"
     return f"create{util.Camelize(ping)}{suffix}"
 
@@ -136,6 +142,12 @@ def output(
                     metrics_list = metrics_by_type.setdefault(metric.type, [])
                     metrics_list.append(metric)
 
+    # Order pings_to_metrics for backwards compatibility with the existing FxA codebase.
+    # Put pings without `event` type metrics first.
+    ping_to_metrics = dict(
+        sorted(ping_to_metrics.items(), key=lambda item: "event" in item[1])
+    )
+
     PING_METRIC_ERROR_MSG = (
         " Server-side environment is simplified and this"
         + " parser doesn't generate individual metric files. Make sure to pass all"

glean_parser/templates/javascript_server.jinja2

@@ -21,7 +21,7 @@ type LoggerOptions = { app: string; fmt?: 'heka' };
 type Event = {
   category: string;
   name: string;
-  extra: Record<string, any>;
+  extra?: Record<string, any>;
   timestamp?: number;
 };
 {% endif %}
@@ -30,14 +30,14 @@ type Event = {
 let _logger{% if lang == "typescript" %}: Logger{% endif %};
 
 {% for ping, metrics_by_type in pings.items() %}
-class {{ ping|event_class_name(event_metric_exists) }} {
+class {{ ping|event_class_name(metrics_by_type) }} {
   {% if lang == "typescript" %}
   _applicationId: string;
   _appDisplayVersion: string;
   _channel: string;
   {% endif %}
   /**
-   * Create {{ ping|event_class_name(event_metric_exists) }} instance.
+   * Create {{ ping|event_class_name(metrics_by_type) }} instance.
    *
    * @param {string} applicationId - The application ID.
    * @param {string} appDisplayVersion - The application display version.
@@ -72,7 +72,7 @@ class {{ ping|event_class_name(event_metric_exists) }} {
       {% endif %}
     }
   }
-  {% if event_metric_exists %}
+  {% if 'event' in metrics_by_type %}
   #record({
   {% else %}
   /**
@@ -99,28 +99,28 @@ class {{ ping|event_class_name(event_metric_exists) }} {
     {% endfor %}
     {% endif %}
     {% endfor %}
-    {% if event_metric_exists %}
+    {% if 'event' in metrics_by_type %}
     event,
     {% endif %}
   {% if lang == "typescript" %}
   }: {
-    user_agent: string,
-    ip_address: string,
+    user_agent: string;
+    ip_address: string;
     {% for metric_type, metrics in metrics_by_type.items() %}
     {% if metric_type != 'event' %}
     {% for metric in metrics %}
-    {{ metric|metric_argument_name }}: {{ metric|js_metric_type }},
+    {{ metric|metric_argument_name }}: {{ metric|js_metric_type }};
     {% endfor %}
     {% endif %}
     {% endfor %}
-    {% if event_metric_exists %}
-    event: Event
+    {% if 'event' in metrics_by_type %}
+    event: Event;
     {% endif %}
   {% endif %}
   }) {
     const now = new Date();
     const timestamp = now.toISOString();
-    {% if event_metric_exists %}
+    {% if 'event' in metrics_by_type %}
     event.timestamp = now.getTime();
     {% endif %}
     const eventPayload = {
@@ -135,7 +135,7 @@ class {{ ping|event_class_name(event_metric_exists) }} {
         {% endif %}
         {% endfor %}
       },
-      {% if event_metric_exists %}
+      {% if 'event' in metrics_by_type %}
       events: [event],
       {% endif %}
       ping_info: {
@@ -171,7 +171,7 @@ class {{ ping|event_class_name(event_metric_exists) }} {
     // this is similar to how FxA currently logs with mozlog: https://github.com/mozilla/fxa/blob/4c5c702a7fcbf6f8c6b1f175e9172cdd21471eac/packages/fxa-auth-server/lib/log.js#L289
     _logger.info(GLEAN_EVENT_MOZLOG_TYPE, ping);
   }
-  {% if event_metric_exists %}
+  {% if 'event' in metrics_by_type %}
   {% for event in metrics_by_type["event"] %}
   /**
    * Record and submit a {{ event.category }}_{{ event.name }} event:
@@ -209,27 +209,27 @@ class {{ ping|event_class_name(event_metric_exists) }} {
     {% endfor %}
   {% if lang == "typescript" %}
   }: {
-    user_agent: string,
-    ip_address: string,
+    user_agent: string;
+    ip_address: string;
     {% for metric_type, metrics in metrics_by_type.items() %}
     {% if metric_type != 'event' %}
     {% for metric in metrics %}
-    {{ metric|metric_argument_name }}: {{ metric|js_metric_type }},
+    {{ metric|metric_argument_name }}: {{ metric|js_metric_type }};
     {% endfor %}
     {% endif %}
     {% endfor %}
     {% for extra, metadata in event.extra_keys.items() %}
-    {{ extra }}: {{metadata.type}},
+    {{ extra }}: {{metadata.type}};
     {% endfor %}
   {% endif %}
   }) {
-    let event = {
-      'category': '{{ event.category }}',
-      'name': '{{ event.name }}',
+    const event = {
+      category: '{{ event.category }}',
+      name: '{{ event.name }}',
       {% if event.extra_keys %}
-      'extra': {
+      extra: {
         {% for extra, metadata in event.extra_keys.items() %}
-        '{{ extra }}': {{ extra }},
+        {{ extra }}: {{ extra }},
         {% endfor %}
       },
       {% endif %}
@@ -244,14 +244,14 @@ class {{ ping|event_class_name(event_metric_exists) }} {
       {% endfor %}
       {% endif %}
       {% endfor %}
-      event
+      event,
     });
   }
   {% endfor %}
   {% endif %}
 }
 {% endfor %}
-{% for ping in pings %}
+{% for ping, metrics_by_type in pings.items() %}
 
 /**
  * Factory function that creates an instance of Glean Server Event Logger to
@@ -262,11 +262,11 @@ class {{ ping|event_class_name(event_metric_exists) }} {
  * @param {Object} logger_options - The logger options.
  * @returns {EventsServerEventLogger} An instance of EventsServerEventLogger.
  */
-export const {{ ping|factory_method(event_metric_exists) }} = function ({
+export const {{ ping|factory_method(metrics_by_type) }} = function ({
   applicationId,
   appDisplayVersion,
   channel,
-  logger_options
+  logger_options,
 {% if lang == "typescript" %}
 }: {
   applicationId: string;
@@ -275,7 +275,7 @@ export const {{ ping|factory_method(event_metric_exists) }} = function ({
   logger_options: LoggerOptions;
 {% endif %}
 }) {
-  return new {{ ping|event_class_name(event_metric_exists) }}(
+  return new {{ ping|event_class_name(metrics_by_type) }}(
     applicationId,
     appDisplayVersion,
     channel,

tests/test_javascript_server.py

@@ -13,6 +13,8 @@ import subprocess
 from glean_parser import javascript_server
 from glean_parser import translate
 from glean_parser import validate_ping
+from glean_parser.metrics import Metric
+from unittest.mock import Mock
 
 
 ROOT = Path(__file__).parent
@@ -65,15 +67,13 @@ def test_parser_js_server(tmp_path):
 def test_generate_ping_factory_method():
     ping = "accounts_events"
     expected_result = "createAccountsEventsEvent"
-    result = javascript_server.generate_ping_factory_method(
-        ping, event_metric_exists=False
-    )
+    result = javascript_server.generate_ping_factory_method(ping, metrics_by_type={})
     assert result == expected_result
 
     ping = "accounts_events"
     expected_result = "createAccountsEventsServerEventLogger"
     result = javascript_server.generate_ping_factory_method(
-        ping, event_metric_exists=True
+        ping, metrics_by_type={"event": [Mock(spec=Metric)]}
     )
     assert result == expected_result