SF-3622 Definitively associate draft generation metrics

Records SF request id to help find related events.

Drafting event metrics that are associated with the same NMT draft
generation request have a 'tags.draftGenerationRequestId' written to
them with an SF-specific id. This id is transferred around by
Activity.Current.Tags, by method arguments, and looked up by finding a
BuildProjectAsync event with a Serval build id.

When SMT is used, `null` is passed for draftGenerationRequestId method
arguments.

The frontend draft-jobs component considers event metrics using both
the older grouping logic as well as a new grouping system which uses
the draftGenerationRequestId to group events. The new system also
considers the BuildCompletedAsync event to be the finishing event for
a job.

A bug represented in the sample data was corrected to show the
incorrect behaviour of recording sf project id in the "userId" field,
rather than recording sf user id as a sf project id.

Author: marksvc

.github/copilot-instructions.md

@@ -42,7 +42,7 @@ This repository contains three interconnected applications:
 
 - Write unit tests for new components and services
 - Follow existing patterns for mocking dependencies
-- Use TestEnvironment pattern from existing tests. Use the TestEnvironment class pattern rather than using a `beforeEach`.
+- Use TestEnvironment pattern from existing tests. Use the TestEnvironment class pattern rather than using a `beforeEach`. Do not put helper functions outside of TestEnvironment classes; helper functions or setup functions should be in the TestEnvironment class.
 - Test both online and offline scenarios
 - Test permission boundaries
 
@@ -53,14 +53,20 @@ This repository contains three interconnected applications:
 - Do not add method comments unless the method would be unclear to an experienced developer.
 - Do put comments into the code to make it more clear what is going on if it would not be obvious to an experienced developer.
 - Do put comments into the code if the intent is not clear from the code.
-- All classes should have a comment to briefly explain why it is there and what its purpose is in the overall system, even if it seems obvious.
-- Please do not fail to add a comment to any classes that are created. All classes should have a comment.
+- All classes and interfaces should have a comment to briefly explain why it is there and what its purpose is in the overall system, even if it seems obvious.
+- Please do not fail to add a comment to any classes or interfaces that are created. All classes and interfaces should have a comment.
 
 # TypeScript language
 
 - Use explicit true/false/null/undefined rather than truthy/falsy
 - Never rely on JavaScript's truthy or falsy. Instead, work with actual true, false, null, and undefined values, rather than relying on their interpretation as truthy or falsy. For example, if `count` might be null, or undefined, or zero, don't write code like `if (count)` or `const foo:string = someVariable ? 'a' : 'b'`. Instead, inspect for the null, undefined, or zero rather than letting the value be interpreted as truthy for falsy. For example, use `if (count == null)` or `const foo:string = someVariable != null 'a' : 'b'` or `if (count > 0)`.
-- Specify types where not obvious, such as when declaring variables and arguments, and for function return types.
+- Specify types when declaring variables, arguments, and for function return types. For example, don't write
+  `const projectId = this.determineProjectId();` or
+  `const buildEvents = eventsSorted.filter(...);` or
+  `const buildEvent = buildEvents[0];`. Instead, write
+  `const projectId: string | undefined = this.determineProjectId();` and
+  `const buildEvents: EventMetric[] = eventsSorted.filter(...);` and
+  `const buildEvent: EventMetric | undefined = buildEvents[0];`.
 - Use `@if {}` syntax rather than `*ngIf` syntax.
 - Although interacting with existing code and APIs may necessitate the use of `null`, when writing new code, prefer using `undefined` rather than `null`.
 - Fields that are of type Subject or BehaviorSubject should have names that end with a `$`.
@@ -72,6 +78,7 @@ This repository contains three interconnected applications:
 - It is better to write code that is verbose and understandable than terse and concise.
 - It is better to explicitly check for and handle problems, or prevent problems from happening, than to assume problems will not happen.
 - Corner-cases happen. They should be handled in code.
+- Please don't change existing code without good justification. Existing code largely works and changing it will cause work for code review. Leave existing code as is when possible.
 
 # Running commands
 

src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metric.ts

@@ -9,6 +9,7 @@ export interface EventMetric {
   timeStamp: string;
   userId?: string;
   executionTime?: string;
+  tags?: { [key: string]: any | undefined };
 }
 
 export enum EventScope {

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.spec.ts

@@ -7,7 +7,7 @@ import { By } from '@angular/platform-browser';
 import { provideNoopAnimations } from '@angular/platform-browser/animations';
 import { ActivatedRoute, provideRouter } from '@angular/router';
 import { BehaviorSubject } from 'rxjs';
-import { anything, mock, when } from 'ts-mockito';
+import { anything, capture, mock, resetCalls, verify, when } from 'ts-mockito';
 import { AuthService } from 'xforge-common/auth.service';
 import { DialogService } from 'xforge-common/dialog.service';
 import { I18nService } from 'xforge-common/i18n.service';
@@ -22,6 +22,7 @@ import { SF_TYPE_REGISTRY } from '../core/models/sf-type-registry';
 import { SFProjectService } from '../core/sf-project.service';
 import { EventMetric, EventScope } from '../event-metrics/event-metric';
 import { DraftJob, DraftJobsComponent } from './draft-jobs.component';
+import { JobDetailsDialogComponent } from './job-details-dialog.component';
 import sampleEvents from './sample-events.json';
 import { ServalAdministrationService } from './serval-administration.service';
 
@@ -94,7 +95,7 @@ describe('DraftJobsComponent', () => {
       expect(draftJob!.startEvent!.id).toEqual('event-metric-05');
       expect(draftJob!.buildEvent!.id).toEqual('event-metric-04');
       expect(draftJob!.cancelEvent).toBeUndefined();
-      expect(draftJob!.finishEvent!.id).toEqual('event-metric-03');
+      expect(draftJob!.finishEvent!.id).toEqual('event-metric-01');
     }));
 
     it('cancelled jobs', fakeAsync(() => {
@@ -109,7 +110,7 @@ describe('DraftJobsComponent', () => {
       expect(draftJob!.startEvent!.id).toEqual('event-metric-14');
       expect(draftJob!.buildEvent!.id).toEqual('event-metric-13');
       expect(draftJob!.cancelEvent!.id).toEqual('event-metric-12');
-      expect(draftJob!.finishEvent).toBeUndefined();
+      expect(draftJob!.finishEvent!.id).toEqual('event-metric-11');
     }));
 
     it('faulted jobs', fakeAsync(() => {
@@ -144,6 +145,227 @@ describe('DraftJobsComponent', () => {
     }));
   });
 
+  describe('associates older events into jobs', () => {
+    // Older events did not have a draftGenerationRequestId and used another association method. They are stored in the sample events file and have an `A` in the various Ids used.
+
+    it('successful jobs', fakeAsync(() => {
+      const env = new TestEnvironment();
+      env.wait();
+      // The Serval build id and event metric ids are written here from looking at the sample data.
+      const servalBuildId = 'serval-build-A1';
+
+      const draftJob: DraftJob = env.component.rows.filter(row => row.job.buildId === servalBuildId)[0].job;
+      // Confirm test setup.
+      expect(draftJob.status).toEqual('success');
+
+      expect(draftJob!.startEvent!.id).toEqual('event-metric-A05');
+      expect(draftJob!.buildEvent!.id).toEqual('event-metric-A04');
+      expect(draftJob!.cancelEvent).toBeUndefined();
+      // Notice that the prior event grouping can use ExecuteWebhookAsync for a finish event rather than BuildCompletedAsync.
+      expect(draftJob!.finishEvent!.id).toEqual('event-metric-A03');
+    }));
+
+    it('cancelled jobs', fakeAsync(() => {
+      const env = new TestEnvironment();
+      env.wait();
+      const servalBuildId = 'serval-build-A2';
+
+      const draftJob: DraftJob = env.component.rows.filter(row => row.job.buildId === servalBuildId)[0].job;
+      // Confirm test setup.
+      expect(draftJob.status).toEqual('cancelled');
+
+      expect(draftJob!.startEvent!.id).toEqual('event-metric-A14');
+      expect(draftJob!.buildEvent!.id).toEqual('event-metric-A13');
+      expect(draftJob!.cancelEvent!.id).toEqual('event-metric-A12');
+      // Notice that the prior event grouping did not define a 'finishEvent' for cancelled jobs.
+      expect(draftJob!.finishEvent).toBeUndefined();
+    }));
+
+    it('faulted jobs', fakeAsync(() => {
+      const env = new TestEnvironment();
+      env.wait();
+      const servalBuildId = 'serval-build-A4';
+
+      const draftJob: DraftJob = env.component.rows.filter(row => row.job.buildId === servalBuildId)[0].job;
+      // Confirm test setup.
+      expect(draftJob.status).toEqual('failed');
+
+      expect(draftJob!.startEvent!.id).toEqual('event-metric-A17');
+      expect(draftJob!.buildEvent!.id).toEqual('event-metric-A16');
+      expect(draftJob!.cancelEvent).toBeUndefined();
+      expect(draftJob!.finishEvent!.id).toEqual('event-metric-A15');
+    }));
+
+    it('in-progress jobs', fakeAsync(() => {
+      // This Serval job had StartPreTranslationBuildAsync and BuildProjectAsync but nothing more yet.
+      const env = new TestEnvironment();
+      env.wait();
+      const servalBuildId = 'serval-build-A5';
+
+      const draftJob: DraftJob = env.component.rows.filter(row => row.job.buildId === servalBuildId)[0].job;
+      // Confirm test setup.
+      expect(draftJob.status).toEqual('running');
+
+      expect(draftJob!.startEvent!.id).toEqual('event-metric-A07');
+      expect(draftJob!.buildEvent!.id).toEqual('event-metric-A06');
+      expect(draftJob!.cancelEvent).toBeUndefined();
+      expect(draftJob!.finishEvent).toBeUndefined();
+    }));
+  });
+
+  describe('job details dialog data', () => {
+    it('includes draft generation request id in dialog data', fakeAsync(() => {
+      const env = new TestEnvironment();
+      env.wait();
+      const jobRow = env.component.rows.find(
+        row => row.job.draftGenerationRequestId != null && row.job.buildId != null
+      );
+
+      expect(jobRow).toBeDefined();
+      if (jobRow == null) {
+        return;
+      }
+
+      resetCalls(mockedDialogService);
+      when(mockedDialogService.openMatDialog(anything(), anything())).thenReturn({} as any);
+
+      env.component.openJobDetailsDialog(jobRow.job);
+
+      verify(mockedDialogService.openMatDialog(anything(), anything())).once();
+      const [component, config] = capture(mockedDialogService.openMatDialog as any).last();
+
+      expect(component).toBe(JobDetailsDialogComponent);
+      const dialogData: any = (config as any)?.data;
+      expect(dialogData.draftGenerationRequestId).toEqual(jobRow.job.draftGenerationRequestId);
+    }));
+  });
+
+  describe('event group validation', () => {
+    it('throws when grouped events contain mismatched request ids', fakeAsync(() => {
+      // Suppose the createJobFromRequestGroup method is called with a set of event metrics that do not have the same draftGenerationRequestId. Throw.
+
+      const env = new TestEnvironment({ hasEvents: false });
+      env.wait();
+      const componentAny: any = env.component;
+      const start = env.createEvent('start-1', 'StartPreTranslationBuildAsync');
+      const build = env.createEvent('build-1', 'BuildProjectAsync', { timeStamp: '2025-01-01T00:01:00.000Z' });
+      const finish = env.createEvent('finish-1', 'BuildCompletedAsync', {
+        timeStamp: '2025-01-01T00:02:00.000Z',
+        payload: { buildState: 'Completed', sfProjectId: 'sf-test-project', buildId: 'build-1' }
+      });
+      const mismatched = env.createEvent('mismatch-1', 'ExecuteWebhookAsync', {
+        tags: { draftGenerationRequestId: 'req-2' },
+        timeStamp: '2025-01-01T00:03:00.000Z'
+      });
+
+      expect(() => componentAny['createJobFromRequestGroup']('req-1', [start, build, finish, mismatched])).toThrowError(
+        /share/i
+      );
+    }));
+
+    it('throws when grouped events contain multiple start events', fakeAsync(() => {
+      const env = new TestEnvironment({ hasEvents: false });
+      env.wait();
+      const componentAny: any = env.component;
+      const startOne = env.createEvent('start-1', 'StartPreTranslationBuildAsync');
+      const startTwo = env.createEvent('start-2', 'StartPreTranslationBuildAsync', {
+        timeStamp: '2025-01-01T00:00:30.000Z'
+      });
+      const build = env.createEvent('build-1', 'BuildProjectAsync', { timeStamp: '2025-01-01T00:01:00.000Z' });
+
+      expect(() => componentAny['createJobFromRequestGroup']('req-1', [startOne, startTwo, build])).toThrowError(
+        /exactly one startpretranslationbuildasync/i
+      );
+    }));
+
+    it('throws when grouped events are missing a start event', fakeAsync(() => {
+      const env = new TestEnvironment({ hasEvents: false });
+      env.wait();
+      const componentAny: any = env.component;
+      const build = env.createEvent('build-1', 'BuildProjectAsync', { timeStamp: '2025-01-01T00:01:00.000Z' });
+
+      expect(() => componentAny['createJobFromRequestGroup']('req-1', [build])).toThrowError(
+        /exactly one startpretranslationbuildasync/i
+      );
+    }));
+
+    it('throws when grouped events contain multiple build events', fakeAsync(() => {
+      const env = new TestEnvironment({ hasEvents: false });
+      env.wait();
+      const componentAny: any = env.component;
+      const start = env.createEvent('start-1', 'StartPreTranslationBuildAsync');
+      const buildOne = env.createEvent('build-1', 'BuildProjectAsync', { timeStamp: '2025-01-01T00:01:00.000Z' });
+      const buildTwo = env.createEvent('build-2', 'BuildProjectAsync', { timeStamp: '2025-01-01T00:01:30.000Z' });
+
+      expect(() => componentAny['createJobFromRequestGroup']('req-1', [start, buildOne, buildTwo])).toThrowError(
+        /more than one buildprojectasync/i
+      );
+    }));
+
+    it('throws when grouped events contain multiple build completed events', fakeAsync(() => {
+      const env = new TestEnvironment({ hasEvents: false });
+      env.wait();
+      const componentAny: any = env.component;
+      const start = env.createEvent('start-1', 'StartPreTranslationBuildAsync');
+      const build = env.createEvent('build-1', 'BuildProjectAsync', { timeStamp: '2025-01-01T00:01:00.000Z' });
+      const finishOne = env.createEvent('finish-1', 'BuildCompletedAsync', {
+        timeStamp: '2025-01-01T00:02:00.000Z',
+        payload: { buildState: 'Completed', sfProjectId: 'sf-test-project', buildId: 'build-1' }
+      });
+      const finishTwo = env.createEvent('finish-2', 'BuildCompletedAsync', {
+        timeStamp: '2025-01-01T00:03:00.000Z',
+        payload: { buildState: 'Completed', sfProjectId: 'sf-test-project', buildId: 'build-1' }
+      });
+
+      expect(() =>
+        componentAny['createJobFromRequestGroup']('req-1', [start, build, finishOne, finishTwo])
+      ).toThrowError(/more than one buildcompletedasync/i);
+    }));
+
+    it('throws when there is a cancel event without a BuildCompleted cancel state', fakeAsync(() => {
+      // When a cancel event happens, the current behavior is that the BuildCompletedAsync event also has a buildState of 'Canceled'. The createJobFromRequestGroup method will expect that.
+
+      const env = new TestEnvironment({ hasEvents: false });
+      env.wait();
+      const componentAny: any = env.component;
+      const start = env.createEvent('start-1', 'StartPreTranslationBuildAsync');
+      const build = env.createEvent('build-1', 'BuildProjectAsync', { timeStamp: '2025-01-01T00:01:00.000Z' });
+      const finish = env.createEvent('finish-1', 'BuildCompletedAsync', {
+        timeStamp: '2025-01-01T00:02:00.000Z',
+        payload: { buildState: 'Completed', sfProjectId: 'sf-test-project', buildId: 'build-1' }
+      });
+      const cancel = env.createEvent('cancel-1', 'CancelPreTranslationBuildAsync', {
+        timeStamp: '2025-01-01T00:02:30.000Z'
+      });
+
+      expect(() => componentAny['createJobFromRequestGroup']('req-1', [start, build, finish, cancel])).toThrowError(
+        /Cancel/i
+      );
+    }));
+  });
+
+  describe('processDraftJobs', () => {
+    it('skips request groups missing a start event', fakeAsync(() => {
+      // Suppose the user selects a dante range such that the start date+time is after a draft generation starts and before it finishes. The list of event metrics will have events about the job being processed, but there will not be a start event. Omit those jobs from the data we present.
+      //
+      // If the end date falls after a start event and before a job's finish event, we'll just show that as in progress.
+
+      const env = new TestEnvironment({ hasEvents: false });
+      env.wait();
+
+      const buildEvent = env.createEvent('build-1', 'BuildProjectAsync', {
+        timeStamp: '2025-01-01T00:01:00.000Z',
+        tags: { draftGenerationRequestId: 'req-1' },
+        result: 'build-1'
+      });
+
+      env.component['draftEvents'] = [buildEvent];
+      env.component['processDraftJobs']();
+      // The job was not included in the data.
+      expect(env.component.rows.length).toBe(0);
+    }));
+  });
+
   describe('duration statistics', () => {
     describe('meanDuration', () => {
       it('should return undefined when rows is empty', fakeAsync(() => {
@@ -435,6 +657,34 @@ class TestEnvironment {
     });
   }
 
+  createEvent(id: string, eventType: string, overrides: Partial<EventMetric> = {}): EventMetric {
+    const event: EventMetric = {
+      id,
+      eventType,
+      timeStamp: '2025-01-01T00:00:00.000Z',
+      scope: EventScope.Drafting,
+      payload: {},
+      userId: 'user-1',
+      projectId: 'sf-test-project',
+      result: undefined,
+      executionTime: undefined,
+      exception: undefined,
+      tags: { draftGenerationRequestId: 'req-1' }
+    };
+
+    if (overrides.timeStamp != null) event.timeStamp = overrides.timeStamp;
+    if (overrides.scope != null) event.scope = overrides.scope;
+    if (overrides.payload != null) event.payload = overrides.payload;
+    if (overrides.userId !== undefined) event.userId = overrides.userId;
+    if (overrides.projectId !== undefined) event.projectId = overrides.projectId;
+    if (overrides.result !== undefined) event.result = overrides.result;
+    if (overrides.executionTime !== undefined) event.executionTime = overrides.executionTime;
+    if (overrides.exception !== undefined) event.exception = overrides.exception;
+    if (overrides.tags != null) event.tags = overrides.tags;
+
+    return event;
+  }
+
   /**
    * Transforms JSON event data to EventMetric objects.
    * Transforms "timeStamp":{"$date":"foo"} to just "timeStamp".
@@ -450,7 +700,8 @@ class TestEnvironment {
       projectId: event.projectId,
       result: event.result,
       executionTime: event.executionTime,
-      exception: event.exception
+      exception: event.exception,
+      tags: event.tags
     }));
   }
 }