From f407341f5f0868306b25edca82a92e49d086ba2d Mon Sep 17 00:00:00 2001 From: Stefan Date: Thu, 10 Sep 2026 21:17:09 +0200 Subject: [PATCH] Parallelize invoice image fetch, add telemetry, generalize HtmlConverter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - InvoiceService.ResolveInlineImagesAsync now fetches an invoice's distinct whitelisted images concurrently (SemaphoreSlim-capped at 4) instead of one at a time. It ran inline on every invoice render, including repeated draft-preview renders, so N sequential mfr-portal round-trips could otherwise add N x latency to a user-facing request. - Add fuchs.invoice.inlineimages.fetches (counter, tagged by outcome: success/http_error/bad_content_type/oversized/exception) and fuchs.invoice.inlineimages.resolve.duration (histogram) to FuchsTelemetry, matching the project's observability convention for outbound-HTTP operations. - MigraDocExtensions.HtmlConverter's pre-resolved-image parameter is renamed inlineImages -> resolvedImages with generic docs: the converter fetches nothing and knows nothing about invoices — which images to resolve, and any whitelist/network policy, is entirely the caller's concern (InvoiceService, ADR 0018 today; any future caller tomorrow). - Add tests asserting fetches actually overlap (not just faster) and that the new counter/histogram are recorded. - Update ADR 0018 to describe the concurrent fetch, telemetry, and the generalized converter parameter. Co-Authored-By: Claude Sonnet 5 --- Fuchs.Tests/InvoiceInlineImageTests.cs | 84 ++++++++++++++++++ ...-invoice-editor-images-inlined-into-pdf.md | 18 ++-- Fuchs/Observability/FuchsTelemetry.cs | 6 ++ Fuchs/Services/InvoiceService.cs | 86 +++++++++++++------ Fuchs/code/MigraDocExtensions.cs | 28 +++--- 5 files changed, 177 insertions(+), 45 deletions(-) diff --git a/Fuchs.Tests/InvoiceInlineImageTests.cs b/Fuchs.Tests/InvoiceInlineImageTests.cs index 363c84a..3a42ee5 100644 --- a/Fuchs.Tests/InvoiceInlineImageTests.cs +++ b/Fuchs.Tests/InvoiceInlineImageTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics.Metrics; using System.Linq; using System.Net; using System.Net.Http; @@ -36,6 +37,34 @@ public class InvoiceInlineImageTests } } + /// Delays each response and records how many requests were in flight at once, so a test + /// can assert fetches actually overlapped instead of inferring it from timing alone. + private sealed class ConcurrencyTrackingHandler : HttpMessageHandler + { + private int _inFlight; + public int MaxObservedConcurrent; + private readonly TimeSpan _delay; + private readonly Func _respond; + public ConcurrencyTrackingHandler(TimeSpan delay, Func respond) + { + _delay = delay; + _respond = respond; + } + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken ct) + { + int now = Interlocked.Increment(ref _inFlight); + InterlockedMax(ref MaxObservedConcurrent, now); + try { await Task.Delay(_delay, ct); return _respond(request); } + finally { Interlocked.Decrement(ref _inFlight); } + } + private static void InterlockedMax(ref int target, int value) + { + int initial; + do { initial = target; if (value <= initial) return; } + while (Interlocked.CompareExchange(ref target, value, initial) != initial); + } + } + private sealed class StubFactory : IHttpClientFactory { private readonly HttpMessageHandler _handler; @@ -216,4 +245,59 @@ public class InvoiceInlineImageTests Assert.Equal(20, result.Count); Assert.Equal(20, handler.RequestedUrls.Count); } + + [Fact] + public async Task ResolveInlineImagesAsync_MultipleImages_FetchesConcurrentlyNotOneAtATime() + { + var handler = new ConcurrencyTrackingHandler(TimeSpan.FromMilliseconds(150), _ => ImageResponse(new byte[] { 1 })); + var svc = MakeService(handler); + var descs = Enumerable.Range(1, 4) + .Select(i => $"") + .ToArray(); + var invoice = InvoiceWithDesc(descs); + + await svc.ResolveInlineImagesAsync(invoice); + + Assert.True(handler.MaxObservedConcurrent > 1, + $"expected overlapping fetches, but max observed concurrency was {handler.MaxObservedConcurrent}"); + } + + [Fact] + public async Task ResolveInlineImagesAsync_Success_RecordsSuccessCounterAndResolveDuration() + { + long fetchOutcomeDelta = 0; + int durationRecords = 0; + using var listener = new MeterListener + { + InstrumentPublished = (inst, l) => + { + if (inst.Meter.Name != "Fuchs.Intranet") return; + if (inst.Name is "fuchs.invoice.inlineimages.fetches" or "fuchs.invoice.inlineimages.resolve.duration") + l.EnableMeasurementEvents(inst); + } + }; + listener.SetMeasurementEventCallback((inst, value, tags, _) => + { + if (inst.Name == "fuchs.invoice.inlineimages.fetches") + foreach (var t in tags) + if (t.Key == "outcome" && (string?)t.Value == "success") + Interlocked.Add(ref fetchOutcomeDelta, value); + }); + listener.SetMeasurementEventCallback((inst, _, _, _) => + { + if (inst.Name == "fuchs.invoice.inlineimages.resolve.duration") + Interlocked.Increment(ref durationRecords); + }); + listener.Start(); + + var handler = new StubHandler(_ => ImageResponse(new byte[] { 1 })); + var svc = MakeService(handler); + string url = svc.AllowedInlineImagePrefix + "pic.png"; + var invoice = InvoiceWithDesc($""); + + await svc.ResolveInlineImagesAsync(invoice); + + Assert.Equal(1, fetchOutcomeDelta); + Assert.True(durationRecords >= 1, "resolve-duration histogram should have recorded one measurement."); + } } diff --git a/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md b/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md index 52e1591..ad4d46b 100644 --- a/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md +++ b/Fuchs/Docs/Decisions/0018-invoice-editor-images-inlined-into-pdf.md @@ -44,15 +44,21 @@ so a naive "download whatever `src` says" would let it reach internal/private ne same library the `HtmlConverter` itself parses the identical text with — rather than a regex, so a decoy attribute next to the real `src` (e.g. `data-src="..."`) can't be picked up in its place. Each fetched image is capped at 5MB, must report an `image/*` content-type, and the invoice - fetches at most 20 distinct images; failures (unreachable, wrong type, oversized) are logged and - just leave that one image out — they never fail the whole render. + fetches at most 20 distinct images; failures (unreachable, wrong type, oversized) are logged, + counted (`FuchsTelemetry.InvoiceInlineImageFetches`, tagged by outcome), and just leave that one + image out — they never fail the whole render. The distinct URLs fetch concurrently (capped at 4 + in flight via a `SemaphoreSlim`, not one-at-a-time) since this runs inline on every render, + including repeated draft-preview renders — total resolve time is recorded in + `FuchsTelemetry.InvoiceInlineImagesResolveDuration`. - The resolved `{src → "base64:..."}` map flows down as a plain `IReadOnlyDictionary?` parameter through `IPdfService.ApplyInvoice` → `FuchsPdf.ApplyInvoice` → `Cell.AddHtml` into the `HtmlConverter`, whose new `img` handler only emits an image for a `src` present in that map — - it does no fetching itself and never sees the network. Image sizing mirrors the HTML: the - handler reads `width`/`height` (attribute or inline `style`, as TinyMCE's resize handles write - them) and converts CSS px → cm, capped to 7×10cm so a malformed/huge size can't blow out the - invoice table layout. + it does no fetching itself and never sees the network. The converter itself stays generic (its + constructor parameter is named `resolvedImages`, not invoice-specific) — resolving *which* images + go in the map, and any network/whitelist policy, is entirely InvoiceService's concern, not the + converter's. Image sizing mirrors the HTML: the handler reads `width`/`height` (attribute or + inline `style`, as TinyMCE's resize handles write them) and converts CSS px → cm, capped to + 7×10cm so a malformed/huge size can't blow out the invoice table layout. ## Consequences - Any future host serving invoice-editor images must be added to the whitelist deliberately (a diff --git a/Fuchs/Observability/FuchsTelemetry.cs b/Fuchs/Observability/FuchsTelemetry.cs index d6708ec..ab78294 100644 --- a/Fuchs/Observability/FuchsTelemetry.cs +++ b/Fuchs/Observability/FuchsTelemetry.cs @@ -54,6 +54,9 @@ public static class FuchsTelemetry public static readonly Counter SystemProbes = Meter.CreateCounter("fuchs.systemstatus.probes", "{probe}", "Admin system-status connectivity probes executed, tagged by component and outcome."); + public static readonly Counter InvoiceInlineImageFetches = + Meter.CreateCounter("fuchs.invoice.inlineimages.fetches", "{fetch}", + "Invoice-text image fetches for inline PDF rendering (ADR 0018), tagged by outcome."); // ── Performance histograms (durations in milliseconds) ─────────────────── public static readonly Histogram PdfRenderDuration = @@ -62,6 +65,9 @@ public static class FuchsTelemetry Meter.CreateHistogram("fuchs.report.render.duration", "ms", "Report render duration."); public static readonly Histogram EmailSendDuration = Meter.CreateHistogram("fuchs.email.send.duration", "ms", "Email send round-trip duration."); + public static readonly Histogram InvoiceInlineImagesResolveDuration = + Meter.CreateHistogram("fuchs.invoice.inlineimages.resolve.duration", "ms", + "Total wall-clock time to fetch an invoice's inline images (all images in parallel)."); /// Starts a span on the application's . public static Activity? StartActivity(string name, ActivityKind kind = ActivityKind.Internal) => diff --git a/Fuchs/Services/InvoiceService.cs b/Fuchs/Services/InvoiceService.cs index ce18178..43ce979 100644 --- a/Fuchs/Services/InvoiceService.cs +++ b/Fuchs/Services/InvoiceService.cs @@ -30,6 +30,7 @@ public class InvoiceService : IInvoiceService private const int MaxInlineImageBytes = 5 * 1024 * 1024; private const int MaxInlineImagesPerInvoice = 20; + private const int MaxConcurrentInlineImageFetches = 4; private readonly Fuchs_intranet _intranet; private readonly IPdfService _pdf; @@ -344,39 +345,68 @@ public class InvoiceService : IInvoiceService } if (urls.Count == 0) return result; + using var act = FuchsTelemetry.StartActivity("invoice.inlineimages.resolve"); + act?.SetTag("fuchs.invoice.id", invoice.Id); + act?.SetTag("fuchs.inlineimages.requested", urls.Count); + var sw = Stopwatch.StartNew(); + var client = _httpClientFactory.CreateClient(InlineImageHttpClientName); - foreach (var url in urls) + // Fetches run concurrently (capped) rather than one-at-a-time: this executes inline on every + // invoice render, including repeated draft-preview renders, so N sequential round-trips to the + // mfr portal would otherwise add N x latency to a user-facing request. + using var gate = new SemaphoreSlim(MaxConcurrentInlineImageFetches); + var fetches = urls.Select(async url => { - try - { - using var resp = await client.GetAsync(url, HttpCompletionOption.ResponseHeadersRead); - if (!resp.IsSuccessStatusCode) - { - _logger.LogWarning("ResolveInlineImagesAsync: {Url} returned {Status}", url, (int)resp.StatusCode); - continue; - } - string contentType = resp.Content.Headers.ContentType?.MediaType ?? ""; - if (!contentType.StartsWith("image/", StringComparison.OrdinalIgnoreCase)) - { - _logger.LogWarning("ResolveInlineImagesAsync: {Url} returned non-image content-type '{ContentType}'", url, contentType); - continue; - } - var bytes = await resp.Content.ReadAsByteArrayAsync(); - if (bytes.Length == 0 || bytes.Length > MaxInlineImageBytes) - { - _logger.LogWarning("ResolveInlineImagesAsync: {Url} image size {Size} out of bounds", url, bytes.Length); - continue; - } - result[url] = FuchsPdf.MigraDocFilenameFromByteArray(bytes); - } - catch (Exception ex) - { - _logger.LogWarning(ex, "ResolveInlineImagesAsync: failed to fetch invoice image {Url}", url); - } - } + await gate.WaitAsync(); + try { return (url, source: await FetchInlineImageAsync(client, url)); } + finally { gate.Release(); } + }); + foreach (var (url, source) in await Task.WhenAll(fetches)) + if (source != null) result[url] = source; + + act?.SetTag("fuchs.inlineimages.resolved", result.Count); + FuchsTelemetry.InvoiceInlineImagesResolveDuration.Record(sw.Elapsed.TotalMilliseconds); return result; } + /// Fetches and validates one whitelisted invoice image; null on any failure (logged + + /// counted, never thrown) so one bad image never fails the whole render. + private async Task FetchInlineImageAsync(HttpClient client, string url) + { + try + { + using var resp = await client.GetAsync(url, HttpCompletionOption.ResponseHeadersRead); + if (!resp.IsSuccessStatusCode) + { + _logger.LogWarning("ResolveInlineImagesAsync: {Url} returned {Status}", url, (int)resp.StatusCode); + FuchsTelemetry.InvoiceInlineImageFetches.Add(1, new KeyValuePair("outcome", "http_error")); + return null; + } + string contentType = resp.Content.Headers.ContentType?.MediaType ?? ""; + if (!contentType.StartsWith("image/", StringComparison.OrdinalIgnoreCase)) + { + _logger.LogWarning("ResolveInlineImagesAsync: {Url} returned non-image content-type '{ContentType}'", url, contentType); + FuchsTelemetry.InvoiceInlineImageFetches.Add(1, new KeyValuePair("outcome", "bad_content_type")); + return null; + } + var bytes = await resp.Content.ReadAsByteArrayAsync(); + if (bytes.Length == 0 || bytes.Length > MaxInlineImageBytes) + { + _logger.LogWarning("ResolveInlineImagesAsync: {Url} image size {Size} out of bounds", url, bytes.Length); + FuchsTelemetry.InvoiceInlineImageFetches.Add(1, new KeyValuePair("outcome", "oversized")); + return null; + } + FuchsTelemetry.InvoiceInlineImageFetches.Add(1, new KeyValuePair("outcome", "success")); + return FuchsPdf.MigraDocFilenameFromByteArray(bytes); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "ResolveInlineImagesAsync: failed to fetch invoice image {Url}", url); + FuchsTelemetry.InvoiceInlineImageFetches.Add(1, new KeyValuePair("outcome", "exception")); + return null; + } + } + public async Task RenderInvoicePdfBytesAsync(FdsInvoiceData invoice, bool draft) { var doc = await GenerateInvoicePdfAsync(invoice, draft); diff --git a/Fuchs/code/MigraDocExtensions.cs b/Fuchs/code/MigraDocExtensions.cs index e038651..c29808b 100644 --- a/Fuchs/code/MigraDocExtensions.cs +++ b/Fuchs/code/MigraDocExtensions.cs @@ -26,13 +26,14 @@ namespace MigraDoc.Extensions public static Section AddHtml(this Section section, string html) => section.Add(html, new MigraDoc.Extensions.Html.HtmlConverter()); - /// Maps an <img> element's original src to a MigraDoc - /// image source ("base64:...") for images already fetched/vetted by the caller (see - /// InvoiceService.ResolveInlineImagesAsync / ADR 0018). Src values not present here are - /// dropped, matching the converter's long-standing behaviour for unsupported elements. + /// Maps an <img> element's original src to a MigraDoc + /// image source ("base64:...") for images the caller has already fetched/vetted — this converter + /// never fetches anything itself. A src not present here is dropped, matching the converter's + /// long-standing behaviour for unsupported elements. (One caller: InvoiceService resolves + /// whitelisted invoice-text image URLs this way — see ADR 0018.) public static Cell AddHtml(this Cell cell, string html, - IReadOnlyDictionary? inlineImages = null) - => cell.Add(html, new MigraDoc.Extensions.Html.HtmlConverter(inlineImages)); + IReadOnlyDictionary? resolvedImages = null) + => cell.Add(html, new MigraDoc.Extensions.Html.HtmlConverter(resolvedImages)); public static TextFrame AddHtml(this TextFrame frame, string html) => frame.Add(html, new MigraDoc.Extensions.Html.HtmlConverter()); @@ -62,15 +63,20 @@ namespace MigraDoc.Extensions namespace MigraDoc.Extensions.Html { - /// Converts a subset of HTML to MigraDoc document objects. + /// + /// Converts a subset of HTML to MigraDoc document objects. Fetches nothing itself: an + /// <img> only renders when its src is a key in the resolvedImages map the + /// caller supplies — any network/whitelist policy for resolving those images lives entirely + /// with the caller (e.g. InvoiceService.ResolveInlineImagesAsync, ADR 0018). + /// public class HtmlConverter : MigraDoc.Extensions.IConverter { private readonly IDictionary> _handlers; - private readonly IReadOnlyDictionary? _inlineImages; + private readonly IReadOnlyDictionary? _resolvedImages; - public HtmlConverter(IReadOnlyDictionary? inlineImages = null) + public HtmlConverter(IReadOnlyDictionary? resolvedImages = null) { - _inlineImages = inlineImages; + _resolvedImages = resolvedImages; _handlers = new Dictionary>(); AddDefaultHandlers(); } @@ -120,7 +126,7 @@ namespace MigraDoc.Extensions.Html _handlers["img"] = (n, p) => { string src = n.GetAttributeValue("src", ""); - if (src.Length == 0 || _inlineImages == null || !_inlineImages.TryGetValue(src, out var source)) + if (src.Length == 0 || _resolvedImages == null || !_resolvedImages.TryGetValue(src, out var source)) return p; var para = GetPara(p); if (para == null) return p;