Parallelize invoice image fetch, add telemetry, generalize HtmlConverter
- 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<string,string>?`
|
||||
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
|
||||
|
||||
@@ -54,6 +54,9 @@ public static class FuchsTelemetry
|
||||
public static readonly Counter<long> SystemProbes =
|
||||
Meter.CreateCounter<long>("fuchs.systemstatus.probes", "{probe}",
|
||||
"Admin system-status connectivity probes executed, tagged by component and outcome.");
|
||||
public static readonly Counter<long> InvoiceInlineImageFetches =
|
||||
Meter.CreateCounter<long>("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<double> PdfRenderDuration =
|
||||
@@ -62,6 +65,9 @@ public static class FuchsTelemetry
|
||||
Meter.CreateHistogram<double>("fuchs.report.render.duration", "ms", "Report render duration.");
|
||||
public static readonly Histogram<double> EmailSendDuration =
|
||||
Meter.CreateHistogram<double>("fuchs.email.send.duration", "ms", "Email send round-trip duration.");
|
||||
public static readonly Histogram<double> InvoiceInlineImagesResolveDuration =
|
||||
Meter.CreateHistogram<double>("fuchs.invoice.inlineimages.resolve.duration", "ms",
|
||||
"Total wall-clock time to fetch an invoice's inline images (all images in parallel).");
|
||||
|
||||
/// <summary>Starts a span on the application's <see cref="ActivitySource"/>.</summary>
|
||||
public static Activity? StartActivity(string name, ActivityKind kind = ActivityKind.Internal) =>
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
/// <summary>Fetches and validates one whitelisted invoice image; null on any failure (logged +
|
||||
/// counted, never thrown) so one bad image never fails the whole render.</summary>
|
||||
private async Task<string?> 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<string, object?>("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<string, object?>("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<string, object?>("outcome", "oversized"));
|
||||
return null;
|
||||
}
|
||||
FuchsTelemetry.InvoiceInlineImageFetches.Add(1, new KeyValuePair<string, object?>("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<string, object?>("outcome", "exception"));
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
public async Task<byte[]> RenderInvoicePdfBytesAsync(FdsInvoiceData invoice, bool draft)
|
||||
{
|
||||
var doc = await GenerateInvoicePdfAsync(invoice, draft);
|
||||
|
||||
@@ -26,13 +26,14 @@ namespace MigraDoc.Extensions
|
||||
|
||||
public static Section AddHtml(this Section section, string html)
|
||||
=> section.Add(html, new MigraDoc.Extensions.Html.HtmlConverter());
|
||||
/// <param name="inlineImages">Maps an <img> element's original <c>src</c> 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.</param>
|
||||
/// <param name="resolvedImages">Maps an <img> element's original <c>src</c> 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.)</param>
|
||||
public static Cell AddHtml(this Cell cell, string html,
|
||||
IReadOnlyDictionary<string, string>? inlineImages = null)
|
||||
=> cell.Add(html, new MigraDoc.Extensions.Html.HtmlConverter(inlineImages));
|
||||
IReadOnlyDictionary<string, string>? 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
|
||||
{
|
||||
/// <summary>Converts a subset of HTML to MigraDoc document objects.</summary>
|
||||
/// <summary>
|
||||
/// Converts a subset of HTML to MigraDoc document objects. Fetches nothing itself: an
|
||||
/// <img> only renders when its <c>src</c> is a key in the <c>resolvedImages</c> map the
|
||||
/// caller supplies — any network/whitelist policy for resolving those images lives entirely
|
||||
/// with the caller (e.g. <c>InvoiceService.ResolveInlineImagesAsync</c>, ADR 0018).
|
||||
/// </summary>
|
||||
public class HtmlConverter : MigraDoc.Extensions.IConverter
|
||||
{
|
||||
private readonly IDictionary<string, Func<HtmlNode, DocumentObject, DocumentObject>> _handlers;
|
||||
private readonly IReadOnlyDictionary<string, string>? _inlineImages;
|
||||
private readonly IReadOnlyDictionary<string, string>? _resolvedImages;
|
||||
|
||||
public HtmlConverter(IReadOnlyDictionary<string, string>? inlineImages = null)
|
||||
public HtmlConverter(IReadOnlyDictionary<string, string>? resolvedImages = null)
|
||||
{
|
||||
_inlineImages = inlineImages;
|
||||
_resolvedImages = resolvedImages;
|
||||
_handlers = new Dictionary<string, Func<HtmlNode, DocumentObject, DocumentObject>>();
|
||||
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;
|
||||
|
||||
Reference in New Issue
Block a user