SF-3622 Definitively associate draft generation metrics

[marksvc]

    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
    considers the BuildCompletedAsync event to be the finishing event for
    a job, but also allows RetrievePreTranslationStatusAsync events with a
    result field equaling the Serval build id to count as a finishing
    event, for easier use in development.
    
    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.
    
    Grouping draft jobs by the draft job generation request id instead of
    timing is optional, to give opportunity to test the new system.

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 85.56701% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.82%. Comparing base (c1d2325) to head (7b03d39).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../app/serval-administration/draft-jobs.component.ts	81.72%	11 Missing and 6 partials ⚠️
...SIL.XForge.Scripture/Services/MachineApiService.cs	88.33%	0 Missing and 7 partials ⚠️
...val-administration/job-details-dialog.component.ts	0.00%	2 Missing ⚠️
...XForge.Scripture/Services/MachineProjectService.cs	80.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3521      +/-   ##
==========================================
+ Coverage   82.76%   82.82%   +0.05%     
==========================================
  Files         610      610              
  Lines       37236    37398     +162     
  Branches     6081     6151      +70     
==========================================
+ Hits        30820    30974     +154     
+ Misses       5498     5478      -20     
- Partials      918      946      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Copilot]

Pull Request Overview

This PR implements tracking of draft generation requests across related NMT drafting events by adding a draftGenerationRequestId tag. This SF-specific identifier is generated at the start of pre-translation builds and propagated through the system using Activity.Current.Tags, method parameters, and event metric lookups.

Key Changes:

• Added Tags field to EventMetric model to store Activity tags
• Modified EventMetricService to collect and save tags from Activity chain (child overrides parent)
• Updated MachineProjectService and MachineApiService to propagate draftGenerationRequestId through method signatures and Activity tags
• For SMT builds, null is passed for draftGenerationRequestId

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
EventMetric.cs	Added Tags dictionary property to store Activity metadata
event-metric.ts	Added optional tags field to TypeScript interface
EventMetricService.cs	Implemented Activity tag collection from parent/child chain with child override logic
EventMetricLogger.cs	Created Activity instances to enable tag propagation through intercepted methods
MachineApiService.cs	Generated draftGenerationRequestId for NMT builds and looked up IDs from BuildProjectAsync events
MachineProjectService.cs	Added draftGenerationRequestId parameter and Activity tag setting for build methods
IMachineProjectService.cs	Updated interface signature with new optional parameter
SyncService.cs	Passed null for draftGenerationRequestId when triggering SMT builds
EventMetricServiceTests.cs	Added tests for Activity tag collection and parent/child override behavior
MachineProjectServiceTests.cs	Updated all test calls with draftGenerationRequestId: null and added Activity tag verification test
MachineApiServiceTests.cs	Added mocks for GetDraftGenerationRequestIdForBuildAsync and tests for tag propagation

[Copilot]

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

[Copilot]

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

[Copilot]

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

[Copilot]

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

[marksvc]

This is on pause so I can learn more about its usage on the frontend.

[marksvc]

Some of the frontend processing and its associated sample data is unfortunately assumptive, since I can not test the Serval process on my workstation as it happens on the server.

[marksvc]

@pmachapman will be a good person to have a look at this PR, as we had discussed design decisions for the backend.

[marksvc]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge/EventMetrics/EventMetric.cs line 83 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

BsonValue should not be nullable, as a null BsonValue is BsonValue.Null, i.e.

 public Dictionary<string, BsonValue>? Tags { get; set; }

Done.

---

src/SIL.XForge/EventMetrics/EventMetricLogger.cs line 103 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This variable name hides Activity activity in the outer scope. Perhaps rename this to Activity currentActivity or similar?

Done.

---

src/SIL.XForge/Services/EventMetricService.cs line 106 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

BsonValue should not be nullable, i.e.

Dictionary<string, BsonValue>? tags = null;

Done.

---

src/SIL.XForge/Services/EventMetricService.cs line 107 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

You will need to change this to use TagObjects (see below)

Dictionary<string, object?> collectedTags = [];

Done.

---

src/SIL.XForge/Services/EventMetricService.cs line 116 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

You should use TagObjects to retrieve not just the string tags, but the other values like ints or objects set via AddTag

Also, this is an inefficient piece of code. Something like this is faster and perhaps clearer?

foreach (KeyValuePair<string, object?> tag in activity.TagObjects)
{
    collectedTags.TryAdd(tag.Key, tag.Value);
}

As an FYI, Tags (i.e. the string values set via AddTag) are passed onto child activities, so would make the while loop surrounding this code redundant. TagObjects contains not just the strings, but any other types set via AddTag, are not passed to child activities, so your while loop is a great idea for this case.

Done. Thanks for pointing that out about string vs objects.

---

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 132 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please add using System.Diagnostics;, and remove this?

Done.

---

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 619 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Here too.

Done.

---

test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs line 582 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please add using System.Diagnostics;, and remove this, and the later occurrences of System.Diagnostics.?

Done.

---

test/SIL.XForge.Scripture.Tests/Services/MachineProjectServiceTests.cs line 589 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please add using System.Diagnostics;, and remove this, and the later occurrences of System.Diagnostics.?

Done.

---

test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs line 407 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please add using System.Diagnostics;, and remove this, and the later occurrences of System.Diagnostics.?

Done.

---

test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs line 409 at r8 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please also test adding an int and a bool value as a tag, and some other object, like a serialized JSON object as a tag? Raymond is likely going to use your work to add tags to record the ServalConfig (which is a JObject).

Done. I'm not 100% sure about whether I should be using BsonDocument.

[pmachapman]

Just one non-blocking nit, so I think it is OK for this to go on to testing.

@pmachapman reviewed 8 of 8 files at r9, all commit messages.
@pmachapman dismissed @github-advanced-security[bot] from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marksvc)

---

test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs line 547 at r9 (raw file):

        EventMetric eventMetric = env.EventMetrics.Query().OrderByDescending(e => e.TimeStamp).First();
        ;

NIT: An extraneous semicolon?

Code quote:

        ;

[marksvc]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marksvc)

---

test/SIL.XForge.Tests/Services/EventMetricServiceTests.cs line 547 at r9 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

NIT: An extraneous semicolon?

Removed.

[pmachapman]

@pmachapman reviewed 2 of 2 files at r10, all commit messages.
@pmachapman dismissed @github-advanced-security[bot] from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @marksvc)

[marksvc]

The new event matching algorithm results in development machines showing builds as never finishing. I relaxed the algorithm to also count RetrievePreTranslationStatusAsync events, with a result equal to the Serval build id, as finish events for a build.

I also added a "Request ID" vs "Timing" selection so we can continue to view builds grouped by the timing algorithm until we are confident about the new event matching code.

I reduced the amount of information looked up by GetDraftGenerationRequestIdForBuildAsync() in case speeding it up helps.

Here is a screenshot of the draft job page. There may be room for aesthetic improvement.

[marksvc]

Hey @pmachapman , I need to toss this back for further review. I made some further modifications as described above.

[pmachapman]

@pmachapman reviewed 11 of 11 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marksvc)