SF-3819 Add support for draft rate limiting

[pmachapman]

This PR adds support for customizable rate limiting for starting Serval draft builds.

If have marked this as testing not required, as to use the rate limiting feature, the database must be modified. As such, it should be tested by a developer.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 96.15385% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.05%. Comparing base (0be7d55) to head (fc50aa0).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...SIL.XForge.Scripture/Services/MachineApiService.cs	93.02%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3974      +/-   ##
==========================================
+ Coverage   81.03%   81.05%   +0.01%     
==========================================
  Files         645      646       +1     
  Lines       41463    41533      +70     
  Branches     6750     6766      +16     
==========================================
+ Hits        33600    33663      +63     
- Misses       6755     6769      +14     
+ Partials     1108     1101       -7     

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

[marksvc]

This is reviewable in Devin Review.

[marksvc]

@marksvc reviewed 10 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on pmachapman).

---

src/SIL.XForge/Models/SiteConfig.cs line 41 at r2 (raw file):

    /// to get total number of hours in the quota period.
    /// </remarks>
    public int BuildQuotaPeriod { get; set; }

Can you tell me some more about the thinking about implementing the build quota timeframes and checking in this way? I've found it to seem a bit awkward and I may not be recalling the right thinking on design or what is needed.

I might have thought we would implement it more like the following. Is that something you were considering as well but found not to be as preferable?

  public int BuildQuotaPerPeriod { get; set; } // eg 12
  public int BuildPeriodHours { get; set; } // eg 24

where we would then be checking if a project is over the quota by considering

count = eventMetrics.countOf(project builds that started in the last BuildPeriodHours hours);
if (count > BuildQuotaPerPeriod)
  throw

A comment in the Jira issue discusses multiple time periods that could be simultaneously limited. So another implementation, that considers multiple timeframes, could be something like

public int BuildQuotaPerDay
public int BuildQuotaPerWeek
public int BuildQuotaPerMonth

and then we would check if a project is over quota by considering

countInDay = eventMetrics.countOf(project builds that started in the last 24 hours);
countInWeek = eventMetrics.countOf(project builds that started in the last 24*7 hours);
countInMonth = eventMetrics.countOf(project builds that started in the last 24*30 hours);
if (countInDay > BuildQuotaPerDay ||
    countInWeek > BuildQuotaPerWeek ||
    countInMonth > BuildQuotaPerMonth)
  throw

I'm at the beginning of reviewing, so by the end I may convinced of usingBuildQuotaPeriod * BuildQuotaPeriodUnit.

---

src/SIL.XForge/Models/SiteConfig.cs line 46 at r2 (raw file):

    /// The quota period unit, stored as a number of hours corresponding to hour, day, week, month, and year.
    /// </summary>
    public QuotaPeriod BuildQuotaPeriodUnit { get; set; }

It appears that BuildQuotaPeriodUnit isn't really interacted with as an enum, but rather as a number. And so usages of it are then made a bit more awkward with serialization or casting. I think if the current implementation of having both BuildQuotaPeriod and BuildQuotaPeriodUnit is desired to move forward with, we could consider removing what may be unnecessarily complication by defining BuildQuotaPeriodUnit as an int rather than as the enum QuotaPeriod type. But perhaps there is more planned that will be using the enum type that I am not foreseeing. And if BuildQuotaPeriodUnit is made to be an int, a preferable name for the property might be BuildQuotaPeriodHours or BuildQuotaPeriodBaseHours.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 282 at r2 (raw file):

          .pipe(
            // No error means build successfully started, so start polling
            switchMap(() => this.pollBuildProgress(buildConfig.projectId))

If the above catchError shows a notice about an error, and we then go into this switchMap and pollBuildProgress, might we get into getBuildProgress and show yet another notice, saying temporarily_unavailable? And so show two notices when unavailable? I'll guess I'll see as soon as I test it.

Some of the mocking in the spec may be hiding some of this behaviour, for example making getBuildProgress return of(undefined) rather than get into its body where it may call noticeService.showError.

Okay I did some debugging and experimenting. In practice, it looks like both MachineApiController.GetBuildAsync and MachineApiController.StartPreTranslationBuildAsync would both need to fail, and with something like a 503 in order for getBuildProgress to also show a message. (Just being over quota is not enough.) And if it did, the result it to show a snackbar and immediately replace it with another. So, it probably isn't your intent that we go into pollBuildProgress even if we went into catchError, but it isn't being a big deal if we do.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 224 at r2 (raw file):

    "preview_last_draft_detail": "The last draft build did not complete, but you can still view the most recent successful draft.",
    "preview_last_draft_header": "Preview last draft",
    "quota_exceeded": "Generating drafts is temporarily unavailable for this project as your quota has been exceeded.",

I wonder if a better message might be to communicate that the quota for the project has been exceeded. For example,
"Generating drafts is temporarily unavailable for this project as its quota has been exceeded."

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2775 at r2 (raw file):

        );
    }

In my testing, I found that sometimes the
buildQuotaLimit,
buildQuotaPeriod, and
buildQuotaPeriodUnit fields in the DB would be all set to 0.

Yeah, try this:

• Set values to 0.
• Generate a draft with project A
• Set values all to 1.
• Generate a draft with project B
• The values are now 0.

---

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

                new SiteOptions
                {
                    Id = "SF",

It looks like from appsettings.json that the value used in the running application would be lower-case "sf", if that would be significant to use here.

Oh I guess it doesn't matter a ton if the SiteConfig Name below is being set to the value here.

[pmachapman]

@pmachapman made 6 comments and resolved 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge/Models/SiteConfig.cs line 41 at r2 (raw file):

Previously, marksvc wrote…

Can you tell me some more about the thinking about implementing the build quota timeframes and checking in this way? I've found it to seem a bit awkward and I may not be recalling the right thinking on design or what is needed.

I might have thought we would implement it more like the following. Is that something you were considering as well but found not to be as preferable?

  public int BuildQuotaPerPeriod { get; set; } // eg 12
  public int BuildPeriodHours { get; set; } // eg 24

where we would then be checking if a project is over the quota by considering

count = eventMetrics.countOf(project builds that started in the last BuildPeriodHours hours);
if (count > BuildQuotaPerPeriod)
  throw

A comment in the Jira issue discusses multiple time periods that could be simultaneously limited. So another implementation, that considers multiple timeframes, could be something like

public int BuildQuotaPerDay
public int BuildQuotaPerWeek
public int BuildQuotaPerMonth

and then we would check if a project is over quota by considering

countInDay = eventMetrics.countOf(project builds that started in the last 24 hours);
countInWeek = eventMetrics.countOf(project builds that started in the last 24*7 hours);
countInMonth = eventMetrics.countOf(project builds that started in the last 24*30 hours);
if (countInDay > BuildQuotaPerDay ||
    countInWeek > BuildQuotaPerWeek ||
    countInMonth > BuildQuotaPerMonth)
  throw

I'm at the beginning of reviewing, so by the end I may convinced of usingBuildQuotaPeriod * BuildQuotaPeriodUnit.

I have reworked this. I think I misread the requirements, and created a solution far more convoluted than it should be. Is it clearer now?

---

src/SIL.XForge/Models/SiteConfig.cs line 46 at r2 (raw file):

Previously, marksvc wrote…

It appears that BuildQuotaPeriodUnit isn't really interacted with as an enum, but rather as a number. And so usages of it are then made a bit more awkward with serialization or casting. I think if the current implementation of having both BuildQuotaPeriod and BuildQuotaPeriodUnit is desired to move forward with, we could consider removing what may be unnecessarily complication by defining BuildQuotaPeriodUnit as an int rather than as the enum QuotaPeriod type. But perhaps there is more planned that will be using the enum type that I am not foreseeing. And if BuildQuotaPeriodUnit is made to be an int, a preferable name for the property might be BuildQuotaPeriodHours or BuildQuotaPeriodBaseHours.

Done. I have removed this property.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 282 at r2 (raw file):

Previously, marksvc wrote…

If the above catchError shows a notice about an error, and we then go into this switchMap and pollBuildProgress, might we get into getBuildProgress and show yet another notice, saying temporarily_unavailable? And so show two notices when unavailable? I'll guess I'll see as soon as I test it.

Some of the mocking in the spec may be hiding some of this behaviour, for example making getBuildProgress return of(undefined) rather than get into its body where it may call noticeService.showError.

Okay I did some debugging and experimenting. In practice, it looks like both MachineApiController.GetBuildAsync and MachineApiController.StartPreTranslationBuildAsync would both need to fail, and with something like a 503 in order for getBuildProgress to also show a message. (Just being over quota is not enough.) And if it did, the result it to show a snackbar and immediately replace it with another. So, it probably isn't your intent that we go into pollBuildProgress even if we went into catchError, but it isn't being a big deal if we do.

Done. I have fixed this to only show an error message once, and not p9oll if the quota is exceeded or some other error occurs.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 224 at r2 (raw file):

Previously, marksvc wrote…

I wonder if a better message might be to communicate that the quota for the project has been exceeded. For example,
"Generating drafts is temporarily unavailable for this project as its quota has been exceeded."

Done. Sounds good.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2775 at r2 (raw file):

Previously, marksvc wrote…

In my testing, I found that sometimes the
buildQuotaLimit,
buildQuotaPeriod, and
buildQuotaPeriodUnit fields in the DB would be all set to 0.

Yeah, try this:

• Set values to 0.
• Generate a draft with project A
• Set values all to 1.
• Generate a draft with project B
• The values are now 0.

Done. I have recreated the bug and added a fix for it.

---

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

Previously, marksvc wrote…

It looks like from appsettings.json that the value used in the running application would be lower-case "sf", if that would be significant to use here.

Oh I guess it doesn't matter a ton if the SiteConfig Name below is being set to the value here.

Done. Fixed - thank you.

[marksvc]

@marksvc reviewed 11 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on pmachapman).

---

src/SIL.XForge/Models/SiteConfig.cs line 41 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I have reworked this. I think I misread the requirements, and created a solution far more convoluted than it should be. Is it clearer now?

Yes, I think it's simpler now.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 282 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done. I have fixed this to only show an error message once, and not p9oll if the quota is exceeded or some other error occurs.

👍

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 224 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done. Sounds good.

👍

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2775 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Done. I have recreated the bug and added a fix for it.

Thank you. I did some more testing.

I just saw my buildQuotaPer* reset from high values back down to all 1s. I wonder if it is setting the values back to something from a previous build when that build fails/completes/something.

I haven't tested this, but the problem may be in MachineBackgroundService.cs ExecuteAsync, where it uses siteConfigs.ReplaceAsync; perhaps it should be more specific in what properties it updates.

---

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

                u.Set(sc => sc.BuildQuotaPerDay, 0);
                u.Set(sc => sc.BuildQuotaPerWeek, 0);
                u.Set(sc => sc.BuildQuotaPerWeek, 0);

Oops, this should be sc.BuildQuotaPerMonth I think.

---

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

                u.Set(sc => sc.BuildQuotaPerDay, 5);
                u.Set(sc => sc.BuildQuotaPerWeek, 20);
                u.Set(sc => sc.BuildQuotaPerWeek, 75);

This, too.

[pmachapman]

@pmachapman made 3 comments and resolved 1 discussion.
Reviewable status: 12 of 14 files reviewed, 3 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2775 at r2 (raw file):

I haven't tested this, but the problem may be in MachineBackgroundService.cs ExecuteAsync, where it uses siteConfigs.ReplaceAsync; perhaps it should be more specific in what properties it updates.

Yes, the issue is there. I thought my fix of loading each loop iteration would work, but you are right, an update is far better. Fixed.

---

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

Previously, marksvc wrote…

Oops, this should be sc.BuildQuotaPerMonth I think.

Done. Thanks!

---

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

Previously, marksvc wrote…

This, too.

Done. Thanks!