mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Add best practices and anti-patterns for Azure integration batching and error classification
This commit is contained in:
parent
07140e2223
commit
01bee2f23f
11 changed files with 210 additions and 237 deletions
|
|
@ -0,0 +1,47 @@
|
|||
// Anti-pattern: "batching" by chaining single calls in one Job Queue run, and
|
||||
// a real batch with no per-item status. Both strand work on one failure.
|
||||
|
||||
codeunit 50150 "WMS Sender (bad)"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
var
|
||||
IntegrationMessage: Record "Integration Message";
|
||||
Client: HttpClient;
|
||||
Request: HttpRequestMessage;
|
||||
Response: HttpResponseMessage;
|
||||
begin
|
||||
IntegrationMessage.SetRange(Status, IntegrationMessage.Status::New);
|
||||
if IntegrationMessage.FindSet() then
|
||||
repeat
|
||||
// BAD: fifty serial round trips inside one task. This is a wait
|
||||
// loop with extra steps: one task and its locks are pinned for the
|
||||
// whole sequence, and a slow remote slows every other queued job.
|
||||
BuildRequest(IntegrationMessage, Request);
|
||||
Client.Send(Request, Response);
|
||||
until IntegrationMessage.Next() = 0;
|
||||
end;
|
||||
|
||||
// BAD alternative: a genuine batch POST whose response is a single status.
|
||||
procedure SendBlindBatch(var Request: HttpRequestMessage; Items: List of [Guid])
|
||||
var
|
||||
Client: HttpClient;
|
||||
Response: HttpResponseMessage;
|
||||
IntegrationMessage: Record "Integration Message";
|
||||
Id: Guid;
|
||||
begin
|
||||
Client.Send(Request, Response);
|
||||
// BAD: one IsSuccessStatusCode for the whole batch. One invalid item
|
||||
// fails all fifty, and we cannot tell which item to fix or retry. A retry
|
||||
// re-sends the items that already succeeded.
|
||||
foreach Id in Items do begin
|
||||
IntegrationMessage.Get(Id);
|
||||
if Response.IsSuccessStatusCode() then
|
||||
IntegrationMessage.Status := IntegrationMessage.Status::Resolved
|
||||
else
|
||||
IntegrationMessage.Status := IntegrationMessage.Status::Failed;
|
||||
IntegrationMessage.Modify(true);
|
||||
end;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
// Best practice: send one bounded batch to a remote that returns per-item
|
||||
// status, carry a per-item idempotency key inside the batch, and settle each
|
||||
// Integration Message individually from its own result. A partial failure parks
|
||||
// only the items that actually failed; a retry of the batch is safe because the
|
||||
// items that already succeeded carry the same keys.
|
||||
|
||||
codeunit 50150 "WMS Batch Sender"
|
||||
{
|
||||
TableNo = "Job Queue Entry";
|
||||
|
||||
trigger OnRun()
|
||||
var
|
||||
IntegrationMessage: Record "Integration Message";
|
||||
BatchBuilder: Codeunit "WMS Batch Builder";
|
||||
BatchSize: Integer;
|
||||
begin
|
||||
// Size is tuned from telemetry, smaller for stages that lock. Not a constant.
|
||||
BatchSize := GetTunedBatchSize();
|
||||
|
||||
IntegrationMessage.SetRange(Direction, IntegrationMessage.Direction::Outbound);
|
||||
IntegrationMessage.SetRange(Status, IntegrationMessage.Status::New);
|
||||
IntegrationMessage.SetRange(Type, 'wms-shipment');
|
||||
if IntegrationMessage.FindSet() then
|
||||
repeat
|
||||
// Each item carries its own Message ID as the idempotency key,
|
||||
// so re-sending the batch never double-processes a sent item.
|
||||
BatchBuilder.AddItem(IntegrationMessage."Message ID", IntegrationMessage);
|
||||
until (IntegrationMessage.Next() = 0) or (BatchBuilder.Count() >= BatchSize);
|
||||
|
||||
// One call. The remote returns a result per item, keyed by Message ID.
|
||||
SendBatchAndSettleEachItem(BatchBuilder);
|
||||
end;
|
||||
|
||||
local procedure SendBatchAndSettleEachItem(var BatchBuilder: Codeunit "WMS Batch Builder")
|
||||
begin
|
||||
// For each per-item result: set that one message Resolved or Failed.
|
||||
// A single bad item parks only itself; the rest move on.
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: integration
|
||||
keywords: [batching, outbound, throughput, partial-failure, per-item-status, telemetry, job-queue]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Batch outbound work only when the remote supports it
|
||||
|
||||
## Description
|
||||
|
||||
Batching several outbound messages into one call reduces per-call overhead, but it couples the fate of the items inside the batch: if the batch of fifty fails, you have to work out which one of the fifty caused it, and most remote APIs return a single success or failure for the whole batch rather than per-item status. Batching is worth it only when the remote exposes a genuine batch endpoint and tells you the outcome of each item, so a single bad item does not strand the other forty-nine. Faking a batch by chaining single calls inside one Job Queue run is worse than not batching at all: it is a synchronous wait loop with extra steps, holding one task and one lock for the whole sequence.
|
||||
|
||||
Batch size is a tuning decision, not a constant. A validate stage can batch large; a posting stage that takes locks should batch small. The right size comes from telemetry on real lock contention and throughput, never from intuition.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Batch in the orchestrator or the sender stage, never inside posting, and only against a remote that accepts a batch and returns per-item status. Keep a per-item idempotency key (the Integration Message id of each item) inside the batch so a retry of the batch does not double-process the items that already succeeded. Mark each message Resolved or Failed individually from the per-item response, so a partial failure parks only the items that actually failed. Start the batch size low and raise it only on the evidence of telemetry, with a smaller size for stages that lock heavily than for stages that only read. See `batch-outbound-work-only-when-the-remote-supports-it.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Two shapes. First, simulated batching: a Job Queue run that loops `HttpClient.Send` over fifty messages to "batch" them, which is a wait loop that pins one task and serialises fifty round trips. Second, blind batching: a real batch POST whose response is a single status with no per-item detail, so one invalid item fails the whole batch and the code cannot tell which item to fix or retry. The detection signal: a loop of `Client.Send` inside one `OnRun`, or a batch send followed by a single `IsSuccessStatusCode` check that flips every message in the batch to the same status. The consequence is that one bad item strands a whole batch and a retry re-sends the items that already succeeded. See `batch-outbound-work-only-when-the-remote-supports-it.bad.al`.
|
||||
|
||||
## See also
|
||||
|
||||
- `send-an-idempotency-key-on-every-outbound-call.md`
|
||||
- `accept-async-work-instead-of-synchronous-wait-loops.md`
|
||||
- `split-multi-step-flows-into-staged-job-queue-entries.md`
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
// Anti-pattern: the failure path records only a raw error string and a Failed
|
||||
// status. There is no error class, so every failure looks the same. On a busy
|
||||
// Monday an operator must open and read three hundred rows to learn that most
|
||||
// were timeouts that would have healed on their own, a handful were bad
|
||||
// addresses, and one was a renamed field that should have paged an engineer.
|
||||
|
||||
codeunit 50140 "Handle Integration Failure"
|
||||
{
|
||||
procedure OnFailure(var IntegrationMessage: Record "Integration Message"; ErrorText: Text)
|
||||
begin
|
||||
IntegrationMessage.Status := IntegrationMessage.Status::Failed;
|
||||
// BAD: raw text, no classification. Nothing tells ops whether to fix
|
||||
// data, wait for the retry, or escalate. The resolution page shows one
|
||||
// undifferentiated Failed bucket and time-to-resolve grows with the queue.
|
||||
IntegrationMessage."Error Message" := CopyStr(ErrorText, 1, MaxStrLen(IntegrationMessage."Error Message"));
|
||||
IntegrationMessage.Modify(true);
|
||||
|
||||
// BAD: a blanket retry of every Failed row, because the code cannot tell
|
||||
// transient from permanent. Data errors and contract breaks are retried
|
||||
// forever, hammering the remote and never reaching a human.
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,39 @@
|
|||
// Best practice: on failure, classify the error into one of three actionable
|
||||
// classes and store it on the Integration Message. Ops then sees a sorted queue
|
||||
// instead of a wall of raw error text. Rules cover the known codes; an AI
|
||||
// classifier (via System.AI) buckets the free-text remainder. The class only
|
||||
// routes the work, it never auto-resolves it.
|
||||
|
||||
enum 50135 "Integration Error Class"
|
||||
{
|
||||
Extensible = true;
|
||||
|
||||
value(0; Unclassified) { Caption = 'Unclassified'; }
|
||||
value(10; DataError) { Caption = 'Data error'; } // a human fixes the payload
|
||||
value(20; Transient) { Caption = 'Transient'; } // the scheduled retry heals it
|
||||
value(30; ContractChange) { Caption = 'Contract change'; } // escalate to the owner
|
||||
}
|
||||
|
||||
codeunit 50140 "Classify Integration Error"
|
||||
{
|
||||
// Called on the failure path, after Status has been set to Failed.
|
||||
procedure Classify(var IntegrationMessage: Record "Integration Message")
|
||||
var
|
||||
AIClassifier: Codeunit "AI Classifier Wrapper";
|
||||
Class: Enum "Integration Error Class";
|
||||
begin
|
||||
// 1) Fast path: deterministic rules over codes we already recognise.
|
||||
Class := ClassifyByKnownCodes(IntegrationMessage."Error Code");
|
||||
|
||||
// 2) Fall back to the AI classifier for the free-text messages rules miss.
|
||||
// The wrapper calls the model through the System.AI module, so the call
|
||||
// is governed and billed, not a raw HttpClient to a model endpoint.
|
||||
if Class = Class::Unclassified then
|
||||
Class := AIClassifier.Classify(IntegrationMessage."Error Message", IntegrationMessage.Type);
|
||||
|
||||
// 3) Store the class so the resolution page can route on it. Advisory only:
|
||||
// a human still confirms a data fix, the retry job still owns transient.
|
||||
IntegrationMessage."Error Class" := Class;
|
||||
IntegrationMessage.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: integration
|
||||
keywords: [error-classification, triage, transient, data-error, contract-change, resolution, ai]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Classify integration errors for resolution
|
||||
|
||||
## Description
|
||||
|
||||
When integration messages fail they pile up, and a single "Failed" status with a raw error string forces an operator to read every one to decide what to do. Most failures fall into one of three classes that demand different responses: a data error (customer not found, invalid currency, wrong VAT code) that a human must fix on the payload, a transient error (timeout, 503, deadlock) that the system should retry with backoff and no human at all, and a contract change (a renamed field, a schema break) that is a code change the integration owner must be paged about. Without the class, transient errors waste human attention while contract breaks sit unescalated, and time-to-resolve grows with the size of the failed queue.
|
||||
|
||||
Classifying each failure and storing the class on the Integration Message turns a Monday pile of three hundred failures into three sorted buckets, each with an obvious next action. The class is one extra field; the value is the routing it enables.
|
||||
|
||||
## Best Practice
|
||||
|
||||
On failure (Status set to Failed with a non-empty error), classify the error into data error, transient, or contract change and write the class to an Error Class field on the Integration Message before ops sees it. The class drives the action: data errors go to the manual resolution page, transient errors are left for the scheduled retry, contract changes raise an alert to the integration owner. A rules table over known error codes handles the common cases; an AI classifier (called through the System.AI module, never a raw model call) buckets the free-text messages that rules miss, which is where most of the time-to-resolve saving comes from. Keep the classifier advisory: the class routes work, it does not auto-resolve it. See `classify-integration-errors-for-resolution.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A failure handler that sets Status to Failed with only a raw error string and no class, leaving an operator to read and triage every row by hand. The detection signal: an integration error path that writes `Error Message` but has no Error Class (or equivalent category) field and no classification step, so the resolution page shows one undifferentiated Failed bucket. The consequence is that retries that would self-heal get manual attention, genuine data fixes wait behind them, and a contract break that should page an engineer looks identical to a transient timeout. See `classify-integration-errors-for-resolution.bad.al`.
|
||||
|
||||
## See also
|
||||
|
||||
- `make-failed-integration-messages-manually-resolvable.md`
|
||||
- `version-business-events-and-keep-payloads-stable.md`
|
||||
- `monitor-external-event-subscription-health.md`
|
||||
|
|
@ -1,29 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: process
|
||||
keywords: [constitution, brief, tech-design, roadmap, project-context, sdd]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Ground work in a solution constitution
|
||||
|
||||
## Description
|
||||
|
||||
A Business Central solution should maintain a small set of durable, high-level documents (the constitution) that every feature spec is grounded in: a project brief (customer and localisation, business processes, goals, non-goals, constraints, success measures), a technical design (architecture, which standard BC modules to reuse, the honest custom-code gaps, the assigned object ID range, the high-level data model, integrations, cross-cutting concerns), and a roadmap (an ordered, numbered feature list with status). These are the documents every agent and engineer reads before doing anything, so individual feature work stays consistent with the agreed direction instead of each feature re-deciding the architecture.
|
||||
|
||||
The constitution exists so that decisions are made once and reused, not re-litigated per feature. The brief fixes the business intent, the technical design fixes the architecture and the object ID range, and the roadmap fixes the order and the numbering that feature folders follow. Because every spec is checked against all three, the documents are where solution-wide consistency actually lives; without them each feature quietly invents its own answer to questions the solution already decided.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Establish the constitution once at the start of a solution, and refresh rather than rewrite it when the business need changes materially, preserving decisions still valid. Interview for missing facts rather than inventing them. Write the brief in plain language with no AL, the technical design favouring reuse of standard BC and justifying every custom-code gap, and the roadmap as a numbered feature list so feature folders match the numbering. Treat the constitution as a human decision: stop for review before proceeding to feature specs. Every feature spec must then be consistent with all three documents.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Specifying or building features with no shared brief, technical design, or roadmap to ground them. The consequence is features that contradict each other on architecture, object ID ranges, or which standard modules to reuse, because each one re-decides in isolation. The signal: feature specs that exist with no constitution behind them, or custom AL introduced with no recorded justification for not reusing standard BC.
|
||||
|
||||
## See also
|
||||
|
||||
- `specify-before-you-build.md`
|
||||
- `map-each-feature-to-an-object-id-range.md`
|
||||
|
|
@ -1,29 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: process
|
||||
keywords: [spec, specification, acceptance-criteria, requirements, sdd, before-implementation]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Specify before you build
|
||||
|
||||
## Description
|
||||
|
||||
Before planning or writing any AL for a Business Central feature, write a feature specification that captures the problem, the users and roles, the scope and out-of-scope edges, the user flow, and testable acceptance criteria. The spec is the what and why; it deliberately names no AL objects. Writing it first grounds the work in agreed requirements and makes the result verifiable: each acceptance criterion is concrete enough to become a test, so "done" is something you can check rather than something you argue about after the code exists.
|
||||
|
||||
Keeping AL out of the spec is deliberate, not an omission. Naming objects too early collapses the what into the how and quietly commits the design before anyone has agreed what the feature must do. The acceptance criteria are the load-bearing part: they are written so each one maps to a single test, which means the spec doubles as the test plan and the definition of done is fixed before any code can drift away from it.
|
||||
|
||||
## Best Practice
|
||||
|
||||
For each feature, produce a spec before the plan and before any code. State the problem and the affected users and roles, draw the scope and out-of-scope boundaries, describe the user flow, and write acceptance criteria concrete enough to turn directly into tests. Record genuinely open items under open questions rather than guessing, and stop for human review of the spec before planning or implementing. Keep AL object names out of the spec; those belong to the planning step. At implementation, confirm every acceptance criterion is covered by a passing test.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Jumping into AL with only an informal idea of the feature and no written, reviewable acceptance criteria. The consequence is scope that drifts during coding, no shared definition of done, and a result that cannot be verified against agreed requirements. The signal: a feature being implemented with no spec, or a spec that lists vague goals instead of testable acceptance criteria, or one that has already committed to AL object names before the what and why are agreed.
|
||||
|
||||
## See also
|
||||
|
||||
- `ground-work-in-a-solution-constitution.md`
|
||||
- `map-each-feature-to-an-object-id-range.md`
|
||||
Loading…
Add table
Add a link
Reference in a new issue