From 9965424cafbd803dedb7094492450d0a4c8cf0c2 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 14:58:47 +0300 Subject: [PATCH 01/27] docs: add tile merge report design spec Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-09-15-tile-merge-report-design.md | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-15-tile-merge-report-design.md diff --git a/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md b/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md new file mode 100644 index 0000000..acb3855 --- /dev/null +++ b/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md @@ -0,0 +1,151 @@ +# Tile Merge Report — Design + +Date: 2026-09-15 +Status: Approved for planning + +## Goal + +Produce a per-task report of what a merge did to the target: + +- **Counts**: added, merged (changed), replaced tiles, plus total. +- **Added-tiles list**: exact `z/x/y` of every tile that did not exist in the target + before the merge. +- Emitted as a **structured log line** (counts + percentages, no list) and a **JSON + artifact file** (full detail incl. the added list). + +Definitions (authoritative): + +- **Added** — tile did not exist in the target before the merge. +- **Merged (changed)** — tile existed and the target pixels were blended with source + pixels (alpha composite). +- **Replaced (full replace)** — tile existed but a fully-opaque source covered it; the + target tile was not blended in. + +## Scope + +- **Per task** (the current execution unit in `TaskExecutor.ExecuteTask`). Job-level + aggregation is a follow-up handled by the job tracker / dashboards. +- No change to merge output tiles themselves — this is observability only. + +## Classification + +Done inside the coord loop of `TaskExecutor.ExecuteTask`, per coord: + +``` +existedBefore = !metadata.IsNewTarget && target.TileExists(coord) // exact coord, no upscale +tile = MergeTiles(builders, coord, strategy, out MergeStats stats) +``` + +`MergeStats` (new, returned by `MergeTiles`): + +- `TargetUsed` — the target tile was included in the flattened image stack. +- `AnySourceUsed` — at least one source tile contributed data. + +Classification: + +| existedBefore | tile | TargetUsed | Result | +|---------------|-------------|------------|----------| +| — | `null` | — | skipped (no write, counted as `skipped`) | +| false | non-null | — | **Added** (record `z/x/y`) | +| true | non-null | true | **Merged** | +| true | non-null | false | **Replaced** | + +Notes / edge cases: + +- `TileExists` is skipped when `IsNewTarget` (target created empty → everything is + Added). This avoids one existence lookup per tile on new targets. +- `TileExists` checks the **exact** coord (no upscale), matching the "exact tile did + not exist" definition. The target builder in the merge uses upscaling, so its + non-null result is not a reliable existence signal — hence the separate check. +- A tile where the target exists but no source contributed data + (`TargetUsed && !AnySourceUsed`) is a target-only re-encode with no real change. + It is counted as `skipped`, not `merged`. (Whether such tiles should be written at + all is pre-existing behavior and out of scope.) + +## Components + +### 1. `MergeStats` + `MergeTiles` signal (MergerLogic/ImageProcessing) + +- Add `MergeStats` struct (`TargetUsed`, `AnySourceUsed`). +- Add `ITileMerger.MergeTiles(..., out MergeStats stats)`. +- Keep the existing `Tile? MergeTiles(...)` as a thin wrapper that discards the stats, + so `MergerCli/Process.cs` and existing `TileMergerTest` are untouched. +- Populate the stats in `GetImageList`: `TargetUsed` = the target builder (index 0) + produced an image that entered the stack; `AnySourceUsed` = any non-target image + entered the stack. Respect the opaque short-circuit and `uploadOnly` (target dropped + → `TargetUsed` always false). + +### 2. `MergeReport` accumulator (MergerService) + +- Plain object created per task. Fields: `jobId`, `taskId`, `taskType`, + `targetFormat`, `isNewTarget`, `startTime`, `endTime`, counts + (`added`, `merged`, `replaced`, `skipped`, `total`), and `List addedTiles`. +- Incremented in the sequential coord loop (no locking needed). +- Computes percentages of `total` per category at finalize. +- Includes a schema `version` field for forward compatibility. + +### 3. `IReportWriter` (MergerService) + +- Serializes the finalized `MergeReport` to JSON and writes it to the configured sink. +- Destination path = `AdditionalParams.ReportOutputPath` on the job object. +- Sink type (S3 vs FS) chosen by service configuration; reuses existing S3/File client + patterns in `MergerLogic/Clients`. +- If `ReportOutputPath` is absent/empty → skip the artifact; still emit the log line. +- Failure to write the artifact must not fail the task — log an error and continue. + +### 4. Wiring + +- Add `ReportOutputPath` (nullable string) to + `MergerService/Models/Jobs/JobParamersAdditiomalParams.cs` (`AdditionalParams`). +- `TaskRunner.RunTask` already reads `job.Parameters.AdditionalParams`; extract + `ReportOutputPath` there and pass it into `ExecuteTask` alongside `managerCallbackUrl`. +- `TaskExecutor.ExecuteTask` builds and finalizes the `MergeReport`, then calls + `IReportWriter` and emits the structured log line. + +## Report JSON shape (artifact) + +```json +{ + "version": 1, + "jobId": "...", + "taskId": "...", + "taskType": "...", + "targetFormat": "PNG", + "isNewTarget": false, + "startTime": "2026-09-15T00:00:00Z", + "endTime": "2026-09-15T00:01:00Z", + "durationSeconds": 60, + "counts": { "added": 10, "merged": 5, "replaced": 2, "skipped": 1, "total": 18 }, + "percentages": { "added": 55.6, "merged": 27.8, "replaced": 11.1, "skipped": 5.6 }, + "addedTiles": [ { "z": 10, "x": 1, "y": 2 } ] +} +``` + +Log line = same object **without** `addedTiles`. + +## Error handling + +- Report writing is best-effort: any exception in serialization/sink write is logged + and swallowed; task success is unaffected. +- Absent `ReportOutputPath` → log line only. + +## Testing + +- Classification branches (added / merged / replaced / skipped) driven by `MergeStats` + and `existedBefore`. +- `MergeStats` population in `TileMerger` (target-only, opaque source, blended, uploadOnly). +- `MergeReport` percentage math and totals. +- `IReportWriter`: FS write, S3 write, absent-path skip, write-failure is non-fatal. + +## Out of scope + +- Job-level aggregation across tasks (tracker/dashboard concern). +- Emitting counts as Prometheus metrics and dashboard work — tracked in a separate + Jira ticket (see below), which also covers auditing/cleaning existing metrics and + organizing the dashboards. + +## Follow-up ticket (drafted, pending confirmation) + +MAPCO ticket to: add added/merged/replaced counts (and percentage-of-total per +category) as Prometheus metrics and to the dashboard; and audit, clean up, and +reorganize existing metrics + dashboards. From b1278666b487bd245f000101c453e6db77b3636c Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:15:41 +0300 Subject: [PATCH 02/27] docs: mark report error-handling as open PR question; link MAPCO-11688 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-09-15-tile-merge-report-design.md | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md b/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md index acb3855..c1ee535 100644 --- a/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md +++ b/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md @@ -123,11 +123,17 @@ Notes / edge cases: Log line = same object **without** `addedTiles`. -## Error handling +## Error handling — OPEN QUESTION (raise on the implementation PR) -- Report writing is best-effort: any exception in serialization/sink write is logged - and swallowed; task success is unaffected. -- Absent `ReportOutputPath` → log line only. +Do **not** bake this in as decided. Post it as an open-question comment on the +implementation PR for reviewer input: + +> Should a failure to write the report artifact (serialization / sink error) be +> best-effort (log + swallow, task still succeeds), or should it fail/reject the task? +> Best-effort is the proposed default, but the report may be a downstream dependency — +> confirm before finalizing. + +- Absent `ReportOutputPath` → log line only (not in question; this is settled). ## Testing @@ -144,8 +150,8 @@ Log line = same object **without** `addedTiles`. Jira ticket (see below), which also covers auditing/cleaning existing metrics and organizing the dashboards. -## Follow-up ticket (drafted, pending confirmation) +## Follow-up ticket -MAPCO ticket to: add added/merged/replaced counts (and percentage-of-total per -category) as Prometheus metrics and to the dashboard; and audit, clean up, and -reorganize existing metrics + dashboards. +**MAPCO-11688** — add added/merged/replaced counts (and percentage-of-total per +category) as Prometheus metrics and to the dashboard; audit, clean up, and reorganize +existing metrics + dashboards. From 3bf450e4fb4a92d0662cc0815703b77144f173e7 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:20:26 +0300 Subject: [PATCH 03/27] docs: add tile merge report implementation plan Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-09-15-tile-merge-report.md | 1109 +++++++++++++++++ 1 file changed, 1109 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-15-tile-merge-report.md diff --git a/docs/superpowers/plans/2026-09-15-tile-merge-report.md b/docs/superpowers/plans/2026-09-15-tile-merge-report.md new file mode 100644 index 0000000..458cb29 --- /dev/null +++ b/docs/superpowers/plans/2026-09-15-tile-merge-report.md @@ -0,0 +1,1109 @@ +# Tile Merge Report Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Produce a per-task report of tiles added / merged / replaced during a merge, emitted as a structured log line (counts + percentages) and a JSON artifact file (full detail incl. exact added `z/x/y`). + +**Architecture:** `TileMerger` reports whether the target and/or any source contributed to each merged tile via a new `MergeStats` out-param. `TaskExecutor` combines that with an exact-coord `TileExists` pre-check to classify every tile, accumulates counts + the added-tile list into a `MergeReport`, then writes a JSON artifact through `IReportWriter` (FS or S3 by config) and logs a summary. Report destination path comes from a new `AdditionalParams.ReportOutputPath` job field. + +**Tech Stack:** C#/.NET, MSTest + Moq (loose `MockRepository`), Newtonsoft.Json, `System.IO.Abstractions` (`IFileSystem`), AWS SDK (`IAmazonS3`). + +**Spec:** `docs/superpowers/specs/2026-09-15-tile-merge-report-design.md` + +--- + +## File Structure + +- Create `MergerLogic/ImageProcessing/MergeStats.cs` — struct carrying `TargetUsed`, `AnySourceUsed`. +- Modify `MergerLogic/ImageProcessing/ITileMerger.cs` — add `MergeTiles(..., out MergeStats stats)` overload. +- Modify `MergerLogic/ImageProcessing/TileMerger.cs` — populate stats; keep old method as wrapper. +- Create `MergerService/Models/Reports/MergeReport.cs` — accumulator + finalize + serialization. +- Create `MergerService/Utils/IReportWriter.cs` + `MergerService/Utils/ReportWriter.cs` — FS/S3 sink writer. +- Modify `MergerService/Models/Jobs/JobParamersAdditiomalParams.cs` — add `ReportOutputPath`. +- Modify `MergerService/Runners/ITaskExecutor.cs` + `TaskExecutor.cs` — classification, accumulation, emit. +- Modify `MergerService/Runners/TaskRunner.cs` — extract `ReportOutputPath`, pass through. +- Modify `MergerService/Program.cs` — register `IReportWriter`. +- Modify `MergerService/appsettings.json` — add `REPORT` config section. +- Tests: `MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs`, `MergerServiceUnitTests/Models/MergeReportTest.cs`, `MergerServiceUnitTests/Utils/ReportWriterTest.cs`, `MergerServiceUnitTests/Runners/TaskExecutorTest.cs`. + +**Global commands** +- Build: `dotnet build GpkgMerger.sln` +- Test one class: `dotnet test --filter "ClassName~"` + +--- + +## Task 1: `MergeStats` struct + +**Files:** +- Create: `MergerLogic/ImageProcessing/MergeStats.cs` + +- [ ] **Step 1: Create the struct** + +```csharp +namespace MergerLogic.ImageProcessing +{ + /// + /// Describes which inputs contributed to a merged tile, used to classify the + /// write as added / merged / replaced. TargetUsed is false in upload-only mode. + /// + public readonly struct MergeStats + { + public bool TargetUsed { get; } + public bool AnySourceUsed { get; } + + public MergeStats(bool targetUsed, bool anySourceUsed) + { + this.TargetUsed = targetUsed; + this.AnySourceUsed = anySourceUsed; + } + } +} +``` + +- [ ] **Step 2: Build** + +Run: `dotnet build GpkgMerger.sln` +Expected: succeeds. + +- [ ] **Step 3: Commit** + +```bash +git add MergerLogic/ImageProcessing/MergeStats.cs +git commit -m "feat: add MergeStats to describe merge tile provenance" +``` + +--- + +## Task 2: `MergeTiles` exposes `MergeStats` + +The target builder is index 0 of the `tiles` list. `GetImageList` iterates from the last source down to the target and short-circuits on the first fully-opaque tile. We must record whether the target image and any source image entered the stack. + +**Files:** +- Modify: `MergerLogic/ImageProcessing/ITileMerger.cs` +- Modify: `MergerLogic/ImageProcessing/TileMerger.cs` +- Test: `MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs` + +- [ ] **Step 1: Write failing tests for the stats out-param** + +Add to `TileMergerTest.cs` (inside the class): + +```csharp +[TestMethod] +[TestCategory("unit")] +[TestCategory("MergeTiles")] +public void MergeTilesStats_BlendedTargetAndSource_TargetAndSourceUsed() +{ + // transparent source over an existing target -> both contribute + var target = new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes()); + var source = new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes()); + var builders = new List { () => target, () => source }; + + this._testTileMerger.MergeTiles(builders, new Coord(0, 0, 0), + new TileFormatStrategy(TileFormat.Png), out MergeStats stats, uploadOnly: false); + + Assert.IsTrue(stats.TargetUsed); + Assert.IsTrue(stats.AnySourceUsed); +} + +[TestMethod] +[TestCategory("unit")] +[TestCategory("MergeTiles")] +public void MergeTilesStats_OpaqueSource_TargetNotUsed() +{ + var target = new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes()); + var opaqueSource = new Tile(new Coord(0, 0, 0), this.GetOpaquePngBytes()); + var builders = new List { () => target, () => opaqueSource }; + + this._testTileMerger.MergeTiles(builders, new Coord(0, 0, 0), + new TileFormatStrategy(TileFormat.Png), out MergeStats stats, uploadOnly: false); + + Assert.IsFalse(stats.TargetUsed); + Assert.IsTrue(stats.AnySourceUsed); +} + +[TestMethod] +[TestCategory("unit")] +[TestCategory("MergeTiles")] +public void MergeTilesStats_UploadOnly_TargetNotUsed() +{ + var target = new Tile(new Coord(0, 0, 0), this.GetOpaquePngBytes()); + var source = new Tile(new Coord(0, 0, 0), this.GetOpaquePngBytes()); + var builders = new List { () => target, () => source }; + + this._testTileMerger.MergeTiles(builders, new Coord(0, 0, 0), + new TileFormatStrategy(TileFormat.Png), out MergeStats stats, uploadOnly: true); + + Assert.IsFalse(stats.TargetUsed); + Assert.IsTrue(stats.AnySourceUsed); +} +``` + +Add these helpers to the test class if not already present (reuse existing test image bytes/fixtures in `Runners/TestData` or the existing `TileMergerTest` fixtures if they already expose transparent/opaque tiles — prefer the existing fixtures and delete these helpers if duplicative): + +```csharp +private byte[] GetTransparentPngBytes() => + File.ReadAllBytes(Path.Combine("TestData", "transparent.png")); +private byte[] GetOpaquePngBytes() => + File.ReadAllBytes(Path.Combine("TestData", "opaque.png")); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `dotnet test --filter "ClassName~TileMergerTest&TestCategory=MergeTiles"` +Expected: FAIL to compile — no `MergeTiles` overload with `out MergeStats`. + +- [ ] **Step 3: Add the interface overload** + +Edit `ITileMerger.cs`: + +```csharp +using MergerLogic.Batching; +using MergerLogic.DataTypes; + +namespace MergerLogic.ImageProcessing +{ + public interface ITileMerger + { + Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, bool uploadOnly = false); + + Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, + out MergeStats stats, bool uploadOnly = false); + } +} +``` + +- [ ] **Step 4: Implement in `TileMerger.cs`** + +Replace the existing `MergeTiles` and `GetImageList` so provenance is tracked. Keep the old signature as a wrapper. + +```csharp +public Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, bool uploadOnly = false) +{ + return this.MergeTiles(tiles, targetCoords, strategy, out _, uploadOnly); +} + +public Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, + out MergeStats stats, bool uploadOnly = false) +{ + bool targetUsed = false; + bool anySourceUsed = false; + + if (uploadOnly) + { + this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] Configured to upload only mode"); + // Ignore target in upload only mode + tiles = tiles.Skip(1).ToList(); + + if (tiles.Count == 1) + { + this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] Only one source was found, using raw image"); + Tile? rawTile = tiles[0](); + rawTile?.ConvertToFormat(strategy.ApplyStrategy(rawTile.Format)); + stats = new MergeStats(false, rawTile != null); + return rawTile; + } + } + + // hasTarget is true when the target builder (index 0) is still part of the list + bool hasTarget = !uploadOnly && tiles.Count > 0; + var images = this.GetImageList(tiles, targetCoords, uploadOnly, hasTarget, out targetUsed, out anySourceUsed); + IMagickImage image; + + switch (images.Count) + { + case 0: + this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] No images where found return null"); + stats = new MergeStats(targetUsed, anySourceUsed); + return null; + case 1: + ImageFormatter.RemoveImageDateAttributes(images[0]); + image = images[0]; + this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] 1 image found"); + break; + default: + using (var imageCollection = new MagickImageCollection()) + { + for (var i = images.Count - 1; i >= 0; i--) + { + imageCollection.Add(images[i]); + } + + this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] {imageCollection.Count} where found for merge, start 'imageMagic' merging"); + using (var mergedImage = imageCollection.Flatten(MagickColor.FromRgba(0, 0, 0, 0))) + { + ImageFormatter.RemoveImageDateAttributes(mergedImage); + mergedImage.ColorSpace = ColorSpace.sRGB; + mergedImage.ColorType = mergedImage.HasAlpha ? ColorType.TrueColorAlpha : ColorType.TrueColor; + image = new MagickImage(mergedImage); + this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] 'imageMagic' merging finished"); + } + } + break; + } + + Tile tile = new Tile(targetCoords, image); + image.Dispose(); + tile.ConvertToFormat(strategy.ApplyStrategy(tile.Format)); + stats = new MergeStats(targetUsed, anySourceUsed); + return tile; +} +``` + +Update `GetImageList` to report provenance. The loop index `i == 0` corresponds to the target when `hasTarget` is true. + +```csharp +private List GetImageList(List tiles, Coord targetCoords, bool uploadOnly, + bool hasTarget, out bool targetUsed, out bool anySourceUsed) +{ + var images = new List(); + int i = tiles.Count - 1; + Tile? tile = null; + targetUsed = false; + anySourceUsed = false; + + bool hasAlpha = false; + try + { + for (; i >= 0; i--) + { + // protect in case all "sources" tiles are null + if (images.Count == 0 && i == 0 && !uploadOnly) + { + this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] All sources are empty - return"); + return images; + } + + tile = tiles[i](); + if (tile is null) + { + continue; + } + + int before = images.Count; + this.AddTileToImageList(targetCoords, tile, images, out hasAlpha); + bool added = images.Count > before; + if (added) + { + if (hasTarget && i == 0) + { + targetUsed = true; + } + else + { + anySourceUsed = true; + } + } + + if (!hasAlpha) + { + return images; + } + } + } + catch + { + images.ForEach(image => image.Dispose()); + throw; + } + + return images; +} +``` + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `dotnet test --filter "ClassName~TileMergerTest&TestCategory=MergeTiles"` +Expected: PASS (all MergeTiles tests, including pre-existing ones). + +- [ ] **Step 6: Commit** + +```bash +git add MergerLogic/ImageProcessing/ITileMerger.cs MergerLogic/ImageProcessing/TileMerger.cs MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs +git commit -m "feat: expose target/source provenance from MergeTiles via MergeStats" +``` + +--- + +## Task 3: `MergeReport` accumulator + +**Files:** +- Create: `MergerService/Models/Reports/MergeReport.cs` +- Test: `MergerServiceUnitTests/Models/MergeReportTest.cs` + +- [ ] **Step 1: Write failing tests** + +Create `MergerServiceUnitTests/Models/MergeReportTest.cs`: + +```csharp +using MergerLogic.DataTypes; +using MergerService.Models.Reports; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Newtonsoft.Json.Linq; +using System; + +namespace MergerServiceUnitTests.Models +{ + [TestClass] + [TestCategory("unit")] + public class MergeReportTest + { + [TestMethod] + public void Counts_And_Percentages_Are_Computed() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordAdded(new Coord(10, 1, 2)); + report.RecordAdded(new Coord(10, 1, 3)); + report.RecordMerged(); + report.RecordReplaced(); + report.RecordSkipped(); + + report.Finalize(new DateTime(2026, 9, 15, 0, 0, 0, DateTimeKind.Utc), + new DateTime(2026, 9, 15, 0, 1, 0, DateTimeKind.Utc)); + + Assert.AreEqual(2, report.Added); + Assert.AreEqual(1, report.Merged); + Assert.AreEqual(1, report.Replaced); + Assert.AreEqual(1, report.Skipped); + Assert.AreEqual(5, report.Total); + Assert.AreEqual(60, report.DurationSeconds); + Assert.AreEqual(40.0, report.AddedPercentage, 0.01); + } + + [TestMethod] + public void Json_Includes_AddedTiles_LogString_Excludes_Them() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordAdded(new Coord(10, 1, 2)); + report.Finalize(DateTime.UnixEpoch, DateTime.UnixEpoch); + + JObject json = JObject.Parse(report.ToJson()); + Assert.AreEqual(1, ((JArray)json["addedTiles"]).Count); + + Assert.IsFalse(report.ToLogString().Contains("addedTiles")); + Assert.IsTrue(report.ToLogString().Contains("\"added\"")); + } + + [TestMethod] + public void Percentages_Are_Zero_When_No_Tiles() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", true); + report.Finalize(DateTime.UnixEpoch, DateTime.UnixEpoch); + Assert.AreEqual(0, report.Total); + Assert.AreEqual(0.0, report.AddedPercentage, 0.01); + } + } +} +``` + +- [ ] **Step 2: Run to verify failure** + +Run: `dotnet test --filter "ClassName~MergeReportTest"` +Expected: FAIL to compile — `MergeReport` does not exist. + +- [ ] **Step 3: Implement `MergeReport`** + +Create `MergerService/Models/Reports/MergeReport.cs`: + +```csharp +using MergerLogic.DataTypes; +using Newtonsoft.Json; + +namespace MergerService.Models.Reports +{ + public class MergeReport + { + public int Version => 1; + public string JobId { get; } + public string TaskId { get; } + public string TaskType { get; } + public string TargetFormat { get; } + public bool IsNewTarget { get; } + + public DateTime StartTime { get; private set; } + public DateTime EndTime { get; private set; } + public double DurationSeconds { get; private set; } + + public int Added { get; private set; } + public int Merged { get; private set; } + public int Replaced { get; private set; } + public int Skipped { get; private set; } + public int Total => this.Added + this.Merged + this.Replaced + this.Skipped; + + public double AddedPercentage { get; private set; } + public double MergedPercentage { get; private set; } + public double ReplacedPercentage { get; private set; } + public double SkippedPercentage { get; private set; } + + public List AddedTiles { get; } = new List(); + + public MergeReport(string jobId, string taskId, string taskType, string targetFormat, bool isNewTarget) + { + this.JobId = jobId; + this.TaskId = taskId; + this.TaskType = taskType; + this.TargetFormat = targetFormat; + this.IsNewTarget = isNewTarget; + } + + public void RecordAdded(Coord coord) + { + this.Added++; + this.AddedTiles.Add(coord); + } + + public void RecordMerged() => this.Merged++; + public void RecordReplaced() => this.Replaced++; + public void RecordSkipped() => this.Skipped++; + + public void Finalize(DateTime startTime, DateTime endTime) + { + this.StartTime = startTime; + this.EndTime = endTime; + this.DurationSeconds = (endTime - startTime).TotalSeconds; + + int total = this.Total; + if (total > 0) + { + this.AddedPercentage = 100.0 * this.Added / total; + this.MergedPercentage = 100.0 * this.Merged / total; + this.ReplacedPercentage = 100.0 * this.Replaced / total; + this.SkippedPercentage = 100.0 * this.Skipped / total; + } + } + + public string ToJson() => JsonConvert.SerializeObject(this, Formatting.None); + + // Summary for the structured log line: counts + percentages, WITHOUT the added-tile list. + public string ToLogString() + { + var summary = new + { + version = this.Version, + jobId = this.JobId, + taskId = this.TaskId, + taskType = this.TaskType, + targetFormat = this.TargetFormat, + isNewTarget = this.IsNewTarget, + durationSeconds = this.DurationSeconds, + counts = new { added = this.Added, merged = this.Merged, replaced = this.Replaced, skipped = this.Skipped, total = this.Total }, + percentages = new { added = this.AddedPercentage, merged = this.MergedPercentage, replaced = this.ReplacedPercentage, skipped = this.SkippedPercentage } + }; + return JsonConvert.SerializeObject(summary, Formatting.None); + } + } +} +``` + +Note: `ToJson()` serializes `AddedTiles` (property name `addedTiles` via camelCase is NOT applied by default in Newtonsoft — the test parses `json["addedTiles"]`). To match, add `[JsonProperty("addedTiles")]` on `AddedTiles` and `[JsonProperty("added")]` etc. only where the test asserts specific names. Concretely add these attributes: + +```csharp +[JsonProperty("addedTiles")] public List AddedTiles { get; } = new List(); +``` + +And in `ToJson()` the top-level `added` count is asserted in the log-string test only (which uses the anonymous object). For `ToJson` the test only checks `addedTiles`, so the single attribute above is sufficient. + +- [ ] **Step 4: Run to verify pass** + +Run: `dotnet test --filter "ClassName~MergeReportTest"` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add MergerService/Models/Reports/MergeReport.cs MergerServiceUnitTests/Models/MergeReportTest.cs +git commit -m "feat: add MergeReport accumulator with counts, percentages and added-tile list" +``` + +--- + +## Task 4: `IReportWriter` + `ReportWriter` (FS/S3 sink) + +Writer serializes a `MergeReport` and writes it to a file named `merge-report-{jobId}-{taskId}.json` under `outputPath`. Sink chosen by config `REPORT:sink` (`"FS"` or `"S3"`). For S3 the bucket comes from `S3:bucket` and `outputPath` is the key prefix. Absent/empty `outputPath` → no-op. Write failures throw (caller decides how to handle — see the open question in the spec). + +**Files:** +- Create: `MergerService/Utils/IReportWriter.cs` +- Create: `MergerService/Utils/ReportWriter.cs` +- Test: `MergerServiceUnitTests/Utils/ReportWriterTest.cs` + +- [ ] **Step 1: Write failing tests** + +Create `MergerServiceUnitTests/Utils/ReportWriterTest.cs`: + +```csharp +using Amazon.S3; +using Amazon.S3.Model; +using MergerLogic.Utils; +using MergerService.Models.Reports; +using MergerService.Utils; +using Microsoft.Extensions.Logging; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using System.IO.Abstractions; +using System.IO.Abstractions.TestingHelpers; +using System.Threading; +using System.Threading.Tasks; + +namespace MergerServiceUnitTests.Utils +{ + [TestClass] + [TestCategory("unit")] + public class ReportWriterTest + { + private Mock _config; + private Mock> _logger; + private Mock _s3; + + [TestInitialize] + public void BeforeEach() + { + this._config = new Mock(MockBehavior.Loose); + this._logger = new Mock>(MockBehavior.Loose); + this._s3 = new Mock(MockBehavior.Loose); + } + + private MergeReport BuildReport() + { + var r = new MergeReport("job1", "task1", "MERGE", "PNG", false); + r.Finalize(System.DateTime.UnixEpoch, System.DateTime.UnixEpoch); + return r; + } + + [TestMethod] + public void FsSink_WritesJsonFile() + { + this._config.Setup(c => c.GetConfiguration("REPORT", "sink")).Returns("FS"); + var fs = new MockFileSystem(); + var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + + writer.WriteReport(this.BuildReport(), "/reports"); + + string expected = fs.Path.Combine("/reports", "merge-report-job1-task1.json"); + Assert.IsTrue(fs.FileExists(expected)); + StringAssert.Contains(fs.File.ReadAllText(expected), "\"jobId\":\"job1\""); + } + + [TestMethod] + public void S3Sink_PutsObject() + { + this._config.Setup(c => c.GetConfiguration("REPORT", "sink")).Returns("S3"); + this._config.Setup(c => c.GetConfiguration("S3", "bucket")).Returns("tiles"); + this._s3.Setup(s => s.PutObjectAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(new PutObjectResponse()); + var fs = new MockFileSystem(); + var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + + writer.WriteReport(this.BuildReport(), "reports"); + + this._s3.Verify(s => s.PutObjectAsync( + It.Is(r => r.BucketName == "tiles" && r.Key == "reports/merge-report-job1-task1.json"), + It.IsAny()), Times.Once); + } + + [TestMethod] + public void EmptyPath_IsNoOp() + { + var fs = new MockFileSystem(); + var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + + writer.WriteReport(this.BuildReport(), null); + writer.WriteReport(this.BuildReport(), ""); + + Assert.AreEqual(0, fs.AllFiles.Count()); + this._s3.Verify(s => s.PutObjectAsync(It.IsAny(), It.IsAny()), Times.Never); + } + } +} +``` + +Ensure the test project references `System.IO.Abstractions.TestingHelpers` (already used elsewhere in the suite; if the package is missing, add `` to `MergerServiceUnitTests.csproj` matching the version of `System.IO.Abstractions` already referenced). + +- [ ] **Step 2: Run to verify failure** + +Run: `dotnet test --filter "ClassName~ReportWriterTest"` +Expected: FAIL to compile — `IReportWriter`/`ReportWriter` do not exist. + +- [ ] **Step 3: Implement interface** + +Create `MergerService/Utils/IReportWriter.cs`: + +```csharp +using MergerService.Models.Reports; + +namespace MergerService.Utils +{ + public interface IReportWriter + { + // Writes the report JSON artifact to the configured sink under outputPath. + // No-op when outputPath is null/empty. Throws on write failure. + void WriteReport(MergeReport report, string? outputPath); + } +} +``` + +- [ ] **Step 4: Implement writer** + +Create `MergerService/Utils/ReportWriter.cs`: + +```csharp +using Amazon.S3; +using Amazon.S3.Model; +using MergerLogic.Utils; +using MergerService.Models.Reports; +using Microsoft.Extensions.Logging; +using System.IO.Abstractions; +using System.Reflection; +using System.Text; + +namespace MergerService.Utils +{ + public class ReportWriter : IReportWriter + { + private readonly IConfigurationManager _configuration; + private readonly IFileSystem _fileSystem; + private readonly IAmazonS3 _s3; + private readonly ILogger _logger; + + public ReportWriter(IConfigurationManager configuration, IFileSystem fileSystem, IAmazonS3 s3, + ILogger logger) + { + this._configuration = configuration; + this._fileSystem = fileSystem; + this._s3 = s3; + this._logger = logger; + } + + public void WriteReport(MergeReport report, string? outputPath) + { + string methodName = MethodBase.GetCurrentMethod().Name; + if (string.IsNullOrEmpty(outputPath)) + { + this._logger.LogDebug($"[{methodName}] No ReportOutputPath configured, skipping report artifact"); + return; + } + + string fileName = $"merge-report-{report.JobId}-{report.TaskId}.json"; + string json = report.ToJson(); + string sink = this._configuration.GetConfiguration("REPORT", "sink"); + + if (string.Equals(sink, "S3", System.StringComparison.OrdinalIgnoreCase)) + { + this.WriteToS3(outputPath, fileName, json); + } + else + { + this.WriteToFs(outputPath, fileName, json); + } + + this._logger.LogInformation($"[{methodName}] Wrote merge report to {sink}:{outputPath}/{fileName}"); + } + + private void WriteToFs(string outputPath, string fileName, string json) + { + this._fileSystem.Directory.CreateDirectory(outputPath); + string fullPath = this._fileSystem.Path.Combine(outputPath, fileName); + this._fileSystem.File.WriteAllText(fullPath, json); + } + + private void WriteToS3(string outputPath, string fileName, string json) + { + string bucket = this._configuration.GetConfiguration("S3", "bucket"); + string key = $"{outputPath.TrimEnd('/')}/{fileName}"; + var request = new PutObjectRequest + { + BucketName = bucket, + Key = key, + ContentBody = json, + ContentType = "application/json" + }; + var res = this._s3.PutObjectAsync(request).Result; + } + } +} +``` + +- [ ] **Step 5: Run to verify pass** + +Run: `dotnet test --filter "ClassName~ReportWriterTest"` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add MergerService/Utils/IReportWriter.cs MergerService/Utils/ReportWriter.cs MergerServiceUnitTests/Utils/ReportWriterTest.cs +git commit -m "feat: add ReportWriter with FS and S3 sinks for merge report artifact" +``` + +--- + +## Task 5: `ReportOutputPath` on `AdditionalParams` + +**Files:** +- Modify: `MergerService/Models/Jobs/JobParamersAdditiomalParams.cs` + +- [ ] **Step 1: Add the nullable field + ctor param** + +Edit `AdditionalParams` to add the property and an optional constructor parameter (optional so existing construction sites and deserialization keep working): + +```csharp +public class AdditionalParams +{ + [JsonInclude] public string? JobTrackerServiceURL { get; } + [JsonInclude] public string? ReportOutputPath { get; } + + [System.Text.Json.Serialization.JsonIgnore] + private JsonSerializerSettings _jsonSerializerSettings; + + public AdditionalParams(string jobTrackerServiceURL, string? reportOutputPath = null) + { + this.JobTrackerServiceURL = jobTrackerServiceURL; + this.ReportOutputPath = reportOutputPath; + + this._jsonSerializerSettings = new JsonSerializerSettings(); + this._jsonSerializerSettings.Converters.Add(new StringEnumConverter()); + } +} +``` + +- [ ] **Step 2: Build** + +Run: `dotnet build GpkgMerger.sln` +Expected: succeeds. + +- [ ] **Step 3: Commit** + +```bash +git add MergerService/Models/Jobs/JobParamersAdditiomalParams.cs +git commit -m "feat: add ReportOutputPath to job AdditionalParams" +``` + +--- + +## Task 6: Classify, accumulate, and emit in `TaskExecutor` + +Add `IReportWriter` dependency, extend `ExecuteTask` with `reportOutputPath`, build a `MergeReport`, classify each tile, then finalize + log + write. + +**Files:** +- Modify: `MergerService/Runners/ITaskExecutor.cs` +- Modify: `MergerService/Runners/TaskExecutor.cs` +- Test: `MergerServiceUnitTests/Runners/TaskExecutorTest.cs` + +- [ ] **Step 1: Write a failing classification test** + +Add to `TaskExecutorTest.cs`. This drives a 1-tile batch where the target already exists and the merged tile uses the target → expect one `RecordMerged` and a report artifact write. Use the existing test's mock wiring for `IDataFactory`/`IData`; add an `IReportWriter` mock and assert it is called once with a report whose `Merged == 1`. + +```csharp +[TestMethod] +[TestCategory("unit")] +[TestCategory("runners")] +public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() +{ + // Arrange: target.TileExists(coord) == true, MergeTiles returns a tile with TargetUsed=true. + var reportWriterMock = this._mockRepository.Create(); + MergeReport captured = null; + reportWriterMock + .Setup(w => w.WriteReport(It.IsAny(), "reports")) + .Callback((r, p) => captured = r); + + var target = new Mock(MockBehavior.Loose); + target.SetupGet(t => t.Type).Returns(DataType.GPKG); + target.Setup(t => t.TileExists(It.IsAny())).Returns(true); + target.Setup(t => t.GetCorrespondingTile(It.IsAny(), It.IsAny())) + .Returns(new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes())); + // ... wire _dataFactoryMock.CreateDataSource(...) to return target for a single-source task + // and metadata with IsNewTarget=false and a single 1x1 batch at z0. + + var executor = this.BuildExecutor(reportWriterMock.Object); // helper that constructs TaskExecutor with all mocks + + // Act + executor.ExecuteTask(BuildSingleTileTask(isNewTarget: false), this._taskUtilsMock.Object, null, "reports"); + + // Assert + reportWriterMock.Verify(w => w.WriteReport(It.IsAny(), "reports"), Times.Once); + Assert.IsNotNull(captured); + Assert.AreEqual(1, captured.Merged); + Assert.AreEqual(0, captured.Added); +} +``` + +Add two sibling tests mirroring this exactly but for the other branches (repeat the arrange block; do not cross-reference): + +- `ExecuteTask_NewTargetTile_CountsAsAdded`: `target.TileExists` returns `false` (or `IsNewTarget=true`), `MergeTiles` yields `AnySourceUsed=true`, `TargetUsed=false`; assert `captured.Added == 1`, `captured.AddedTiles.Count == 1`. +- `ExecuteTask_OpaqueSourceOverExisting_CountsAsReplaced`: `target.TileExists` returns `true`, `MergeTiles` yields `TargetUsed=false`, `AnySourceUsed=true`; assert `captured.Replaced == 1`. + +To make the merge outcome deterministic in these tests, mock `ITileMerger` instead of using the real one, so the test controls the returned `MergeStats`: + +```csharp +this._tileMergerMock = this._mockRepository.Create(); +MergeStats outStats = new MergeStats(targetUsed: false, anySourceUsed: true); +this._tileMergerMock + .Setup(m => m.MergeTiles(It.IsAny>(), It.IsAny(), + It.IsAny(), out outStats, It.IsAny())) + .Returns(new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes())); +``` + +(Replace the currently-used real `_testTileMerger` in the executor construction with `_tileMergerMock.Object` for these tests.) + +- [ ] **Step 2: Run to verify failure** + +Run: `dotnet test --filter "ClassName~TaskExecutorTest&TestCategory=runners"` +Expected: FAIL to compile — `ExecuteTask` has no `reportOutputPath` param and `TaskExecutor` ctor has no `IReportWriter`. + +- [ ] **Step 3: Update the interface** + +Edit `ITaskExecutor.cs`: + +```csharp +using MergerService.Models.Tasks; +using MergerService.Utils; + +namespace MergerService.Runners +{ + public interface ITaskExecutor + { + void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl, string? reportOutputPath); + } +} +``` + +- [ ] **Step 4: Wire the writer + report into `TaskExecutor`** + +In `TaskExecutor.cs`: + +1. Add field + ctor param: + +```csharp +private readonly IReportWriter _reportWriter; +``` + +Add `IReportWriter reportWriter` to the constructor signature and assign `this._reportWriter = reportWriter;`. + +2. Add `using MergerService.Models.Reports;` at the top. + +3. Change the method signature: + +```csharp +public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl, string? reportOutputPath) +``` + +4. After `MergeMetadata metadata = task.Parameters;` create the report and capture start time: + +```csharp +DateTime reportStart = DateTime.UtcNow; +MergeReport report = new MergeReport(task.JobId, task.Id, task.Type, + metadata.TargetFormat.ToString(), metadata.IsNewTarget); +``` + +5. Replace the per-coord merge block. Currently: + +```csharp +var tileMergeStopwatch = Stopwatch.StartNew(); +Tile? tile = this._tileMerger.MergeTiles(correspondingTileBuilders, coord, strategy, metadata.IsNewTarget); +tileMergeStopwatch.Stop(); +this._metricsProvider.MergeTimePerTileHistogram(tileMergeStopwatch.Elapsed.TotalSeconds, metadata.TargetFormat); + +if (tile != null) +{ + tiles.Add(tile); + currentBatchBytes += tile.Size(); + ... +} +``` + +Change to classify before/after merge: + +```csharp +bool existedBefore = !metadata.IsNewTarget && target.TileExists(coord); + +var tileMergeStopwatch = Stopwatch.StartNew(); +Tile? tile = this._tileMerger.MergeTiles(correspondingTileBuilders, coord, strategy, out MergeStats stats, metadata.IsNewTarget); +tileMergeStopwatch.Stop(); +this._metricsProvider.MergeTimePerTileHistogram(tileMergeStopwatch.Elapsed.TotalSeconds, metadata.TargetFormat); + +if (tile != null) +{ + if (!stats.AnySourceUsed) + { + // target re-encode with no source data — not a real change + report.RecordSkipped(); + } + else if (!existedBefore) + { + report.RecordAdded(coord); + } + else if (stats.TargetUsed) + { + report.RecordMerged(); + } + else + { + report.RecordReplaced(); + } + + tiles.Add(tile); + currentBatchBytes += tile.Size(); + + if (currentBatchBytes >= this._batchMaxBytes || (this._limitBatchSize && tiles.Count >= this._batchMaxSize)) + { + this.UpdateTargetTiles(target, tiles, task, overallTileProgressCount, totalTileCount, taskUtils); + tiles.Clear(); + currentBatchBytes = 0; + } +} +else +{ + report.RecordSkipped(); +} +``` + +6. After `target.Wrapup();` (still inside `ExecuteTask`, before the final debug log), finalize + emit: + +```csharp +report.Finalize(reportStart, DateTime.UtcNow); +this._logger.LogInformation($"[{methodName}] Merge report: {report.ToLogString()}"); +try +{ + this._reportWriter.WriteReport(report, reportOutputPath); +} +catch (Exception e) +{ + // Best-effort (proposed default). Whether this should fail the task is an open + // question raised on the implementation PR. + this._logger.LogError(e, $"[{methodName}] Failed to write merge report artifact: {e.Message}"); +} +``` + +- [ ] **Step 5: Run to verify pass** + +Run: `dotnet test --filter "ClassName~TaskExecutorTest&TestCategory=runners"` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add MergerService/Runners/ITaskExecutor.cs MergerService/Runners/TaskExecutor.cs MergerServiceUnitTests/Runners/TaskExecutorTest.cs +git commit -m "feat: classify added/merged/replaced tiles and emit merge report in TaskExecutor" +``` + +--- + +## Task 7: Pass `ReportOutputPath` through `TaskRunner` + +**Files:** +- Modify: `MergerService/Runners/TaskRunner.cs` +- Test: `MergerServiceUnitTests/Runners/TaskRunnerTest.cs` + +- [ ] **Step 1: Write/adjust failing test** + +`RunTask` currently calls `this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl)`. The `ITaskExecutor` change (Task 6) breaks compilation of `TaskRunner` and its tests. Update the `TaskRunnerTest` verification of `ExecuteTask` to include the new argument, and add an assertion that the `ReportOutputPath` from the job flows through: + +```csharp +// In the existing "happy path" RunTask test where a job is returned by _jobUtils: +this._taskExecutorMock.Verify(e => e.ExecuteTask(task, this._taskUtils, It.IsAny(), "reports"), Times.Once); +``` + +Set the mocked job's `AdditionalParams.ReportOutputPath` to `"reports"` in that test's job fixture. + +- [ ] **Step 2: Run to verify failure** + +Run: `dotnet test --filter "ClassName~TaskRunnerTest"` +Expected: FAIL (compile or verification mismatch). + +- [ ] **Step 3: Implement pass-through** + +In `TaskRunner.RunTask`, where `managerCallbackUrl` is derived, also derive the report path from the same job, then pass it: + +```csharp +MergeJob? job = this._jobUtils.GetJob(task.JobId); +string? managerCallbackUrl = job?.Parameters.AdditionalParams?.JobTrackerServiceURL; +string? reportOutputPath = job?.Parameters.AdditionalParams?.ReportOutputPath; +``` + +(There is currently a single inline `this._jobUtils.GetJob(task.JobId)?...` call for `managerCallbackUrl`; replace it with the `job` local above so the job is fetched once.) Then: + +```csharp +this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl, reportOutputPath); +``` + +Add `using MergerService.Models.Jobs;` if not already present. + +- [ ] **Step 4: Run to verify pass** + +Run: `dotnet test --filter "ClassName~TaskRunnerTest"` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add MergerService/Runners/TaskRunner.cs MergerServiceUnitTests/Runners/TaskRunnerTest.cs +git commit -m "feat: pass job ReportOutputPath from TaskRunner to TaskExecutor" +``` + +--- + +## Task 8: Register `IReportWriter` + add `REPORT` config + +**Files:** +- Modify: `MergerService/Program.cs` +- Modify: `MergerService/appsettings.json` + +- [ ] **Step 1: Register the writer** + +In `Program.cs`, after `builder.Services.AddSingleton();` add: + +```csharp +builder.Services.AddSingleton(); +``` + +Add `using MergerService.Utils;` if not already imported. `IFileSystem`, `IAmazonS3`, and `IConfigurationManager` are already registered via `RegisterMergerLogicType()`. + +- [ ] **Step 2: Add config section** + +In `appsettings.json`, add a top-level `REPORT` section: + +```json + "REPORT": { + "sink": "FS" + }, +``` + +- [ ] **Step 3: Build** + +Run: `dotnet build GpkgMerger.sln` +Expected: succeeds. + +- [ ] **Step 4: Commit** + +```bash +git add MergerService/Program.cs MergerService/appsettings.json +git commit -m "build: register ReportWriter and add REPORT sink config" +``` + +--- + +## Task 9: Full build + test sweep + +**Files:** none. + +- [ ] **Step 1: Build the solution** + +Run: `dotnet build GpkgMerger.sln` +Expected: succeeds with no errors. + +- [ ] **Step 2: Run the full unit-test suite** + +Run: `dotnet test GpkgMerger.sln --filter "TestCategory=unit"` +Expected: all tests pass, including the pre-existing `TileMergerTest`, `TaskExecutorTest`, and `TaskRunnerTest`. + +- [ ] **Step 3: Verify the CLI still compiles against `ITileMerger`** + +`MergerCli/Process.cs` uses the original `MergeTiles(...)` overload, which is preserved. Confirm it builds (covered by Step 1). No change required. + +- [ ] **Step 4: Post the open question on the PR** + +After opening the implementation PR, post the error-handling open question from the spec (`## Error handling — OPEN QUESTION`) as a PR comment so reviewers decide whether report-write failure should stay best-effort or fail the task. + +--- + +## Post-implementation + +- Follow-up ticket **MAPCO-11688** covers exposing these counts as Prometheus metrics + dashboard and auditing/reorganizing existing metrics/dashboards. Not part of this plan. From 463732365b15b1639630e35de01be970090122d9 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:27:13 +0300 Subject: [PATCH 04/27] docs: note existing TaskExecutorTest call sites to update in plan Task 6 Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/superpowers/plans/2026-09-15-tile-merge-report.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/superpowers/plans/2026-09-15-tile-merge-report.md b/docs/superpowers/plans/2026-09-15-tile-merge-report.md index 458cb29..c6df84d 100644 --- a/docs/superpowers/plans/2026-09-15-tile-merge-report.md +++ b/docs/superpowers/plans/2026-09-15-tile-merge-report.md @@ -865,6 +865,11 @@ namespace MergerService.Runners } ``` +**IMPORTANT — existing call sites to update (compile breakers):** +- `MergerServiceUnitTests/Runners/TaskExecutorTest.cs` constructs `TaskExecutor` in **two** places (in `WhenGivenSourcesWithOneTile...` and `WhenConfiguringBatchLimits...`). Add an `IReportWriter` mock in `BeforeEach` and pass `reportWriterMock.Object` as the new final ctor arg at both sites. +- The same two tests call `ExecuteTask(testTask, _taskUtilsMock.Object, null)` — add the 4th arg → `ExecuteTask(testTask, _taskUtilsMock.Object, null, null)`. +- `MockRepository` is `Loose` and there is no `VerifyAll()`, so the new `target.TileExists(coord)` call in `ExecuteTask` (unmocked → returns `false`) does not break these existing tests. + - [ ] **Step 4: Wire the writer + report into `TaskExecutor`** In `TaskExecutor.cs`: From ca4287cdbf02473cbcd6ae6fe6fda59b697dfbc1 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:28:22 +0300 Subject: [PATCH 05/27] feat: add MergeStats to describe merge tile provenance Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerLogic/ImageProcessing/MergeStats.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 MergerLogic/ImageProcessing/MergeStats.cs diff --git a/MergerLogic/ImageProcessing/MergeStats.cs b/MergerLogic/ImageProcessing/MergeStats.cs new file mode 100644 index 0000000..4fee673 --- /dev/null +++ b/MergerLogic/ImageProcessing/MergeStats.cs @@ -0,0 +1,18 @@ +namespace MergerLogic.ImageProcessing +{ + /// + /// Describes which inputs contributed to a merged tile, used to classify the + /// write as added / merged / replaced. TargetUsed is false in upload-only mode. + /// + public readonly struct MergeStats + { + public bool TargetUsed { get; } + public bool AnySourceUsed { get; } + + public MergeStats(bool targetUsed, bool anySourceUsed) + { + this.TargetUsed = targetUsed; + this.AnySourceUsed = anySourceUsed; + } + } +} From 7bf3f63f812a700f508e2d966aaf8374d0e84127 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:32:31 +0300 Subject: [PATCH 06/27] feat: expose target/source provenance from MergeTiles via MergeStats Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerLogic/ImageProcessing/ITileMerger.cs | 2 + MergerLogic/ImageProcessing/TileMerger.cs | 33 ++++++++++- .../ImageProcessing/TileMergerTest.cs | 59 +++++++++++++++++++ 3 files changed, 92 insertions(+), 2 deletions(-) diff --git a/MergerLogic/ImageProcessing/ITileMerger.cs b/MergerLogic/ImageProcessing/ITileMerger.cs index b1bd461..749072c 100644 --- a/MergerLogic/ImageProcessing/ITileMerger.cs +++ b/MergerLogic/ImageProcessing/ITileMerger.cs @@ -6,5 +6,7 @@ namespace MergerLogic.ImageProcessing public interface ITileMerger { Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, bool uploadOnly = false); + + Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, out MergeStats stats, bool uploadOnly = false); } } diff --git a/MergerLogic/ImageProcessing/TileMerger.cs b/MergerLogic/ImageProcessing/TileMerger.cs index 4a9a5a2..0f0ca14 100644 --- a/MergerLogic/ImageProcessing/TileMerger.cs +++ b/MergerLogic/ImageProcessing/TileMerger.cs @@ -20,6 +20,15 @@ public TileMerger(ITileScaler tileScaler, ILogger logger) public Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, bool uploadOnly = false) { + return this.MergeTiles(tiles, targetCoords, strategy, out _, uploadOnly); + } + + public Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, + out MergeStats stats, bool uploadOnly = false) + { + bool targetUsed = false; + bool anySourceUsed = false; + if(uploadOnly) { this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] Configured to upload only mode"); // Ignore target if in upload only mode @@ -30,11 +39,13 @@ public TileMerger(ITileScaler tileScaler, ILogger logger) this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] Only one source was found, using raw image"); Tile? rawTile = tiles[0](); rawTile?.ConvertToFormat(strategy.ApplyStrategy(rawTile.Format)); + stats = new MergeStats(false, rawTile != null); return rawTile; } } - var images = this.GetImageList(tiles, targetCoords, uploadOnly); + bool hasTarget = !uploadOnly && tiles.Count > 0; + var images = this.GetImageList(tiles, targetCoords, uploadOnly, hasTarget, out targetUsed, out anySourceUsed); IMagickImage image; switch (images.Count) @@ -42,6 +53,7 @@ public TileMerger(ITileScaler tileScaler, ILogger logger) case 0: // There are no images this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] No images where found return null"); + stats = new MergeStats(targetUsed, anySourceUsed); return null; case 1: ImageFormatter.RemoveImageDateAttributes(images[0]); @@ -73,14 +85,18 @@ public TileMerger(ITileScaler tileScaler, ILogger logger) Tile tile = new Tile(targetCoords, image); image.Dispose(); tile.ConvertToFormat(strategy.ApplyStrategy(tile.Format)); + stats = new MergeStats(targetUsed, anySourceUsed); return tile; } - private List GetImageList(List tiles, Coord targetCoords, bool uploadOnly) + private List GetImageList(List tiles, Coord targetCoords, bool uploadOnly, + bool hasTarget, out bool targetUsed, out bool anySourceUsed) { var images = new List(); int i = tiles.Count - 1; Tile? tile = null; + targetUsed = false; + anySourceUsed = false; bool hasAlpha = false; try @@ -100,7 +116,20 @@ private List GetImageList(List tiles, Coo continue; } + int before = images.Count; this.AddTileToImageList(targetCoords, tile, images, out hasAlpha); + if (images.Count > before) + { + if (hasTarget && i == 0) + { + targetUsed = true; + } + else + { + anySourceUsed = true; + } + } + if (!hasAlpha) { return images; diff --git a/MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs b/MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs index d8cde85..1be3cc9 100644 --- a/MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs +++ b/MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs @@ -339,6 +339,65 @@ public void MergeTiles(Tile[] tiles, Coord targetCoord, TileFormatStrategy strat CollectionAssert.AreEqual(expectedTileBytes, result.GetImageBytes()); } + [TestMethod] + [TestCategory("MergeTiles")] + public void MergeTilesStatsBlendedTargetAndSource() + { + var targetCoord = new Coord(15, 0, 0); + // target (index 0) is transparent, source (last) is transparent -> both enter the stack + var tiles = new[] + { + new Tile(targetCoord, File.ReadAllBytes("2.png")), + new Tile(targetCoord, File.ReadAllBytes("1.png")) + }; + var tileBuilders = tiles.Select(tile => () => tile).ToList(); + + var result = this._testTileMerger.MergeTiles(tileBuilders, targetCoord, new TileFormatStrategy(TileFormat.Png), out var stats); + + Assert.IsNotNull(result); + Assert.IsTrue(stats.TargetUsed); + Assert.IsTrue(stats.AnySourceUsed); + } + + [TestMethod] + [TestCategory("MergeTiles")] + public void MergeTilesStatsOpaqueSourceOverTarget() + { + var targetCoord = new Coord(15, 0, 0); + // opaque source (last) short-circuits before the target (index 0) is reached + var tiles = new[] + { + new Tile(targetCoord, File.ReadAllBytes("1.png")), + new Tile(targetCoord, File.ReadAllBytes("3.jpeg")) + }; + var tileBuilders = tiles.Select(tile => () => tile).ToList(); + + var result = this._testTileMerger.MergeTiles(tileBuilders, targetCoord, new TileFormatStrategy(TileFormat.Jpeg), out var stats); + + Assert.IsNotNull(result); + Assert.IsFalse(stats.TargetUsed); + Assert.IsTrue(stats.AnySourceUsed); + } + + [TestMethod] + [TestCategory("MergeTiles")] + public void MergeTilesStatsUploadOnly() + { + var targetCoord = new Coord(15, 0, 0); + var tiles = new[] + { + new Tile(targetCoord, File.ReadAllBytes("2.png")), + new Tile(targetCoord, File.ReadAllBytes("1.png")) + }; + var tileBuilders = tiles.Select(tile => () => tile).ToList(); + + var result = this._testTileMerger.MergeTiles(tileBuilders, targetCoord, new TileFormatStrategy(TileFormat.Jpeg), out var stats, uploadOnly: true); + + Assert.IsNotNull(result); + Assert.IsFalse(stats.TargetUsed); + Assert.IsTrue(stats.AnySourceUsed); + } + #endregion } } From ec9362685b25dc5e07ac004db86db500cc708d6a Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:38:08 +0300 Subject: [PATCH 07/27] feat: add MergeReport accumulator with counts, percentages and added-tile list Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Models/Reports/MergeReport.cs | 88 +++++++++++++++++++ .../Models/MergeReportTest.cs | 58 ++++++++++++ 2 files changed, 146 insertions(+) create mode 100644 MergerService/Models/Reports/MergeReport.cs create mode 100644 MergerServiceUnitTests/Models/MergeReportTest.cs diff --git a/MergerService/Models/Reports/MergeReport.cs b/MergerService/Models/Reports/MergeReport.cs new file mode 100644 index 0000000..7abea1c --- /dev/null +++ b/MergerService/Models/Reports/MergeReport.cs @@ -0,0 +1,88 @@ +using MergerLogic.DataTypes; +using Newtonsoft.Json; + +namespace MergerService.Models.Reports +{ + public class MergeReport + { + public int Version => 1; + public string JobId { get; } + public string TaskId { get; } + public string TaskType { get; } + public string TargetFormat { get; } + public bool IsNewTarget { get; } + + public DateTime StartTime { get; private set; } + public DateTime EndTime { get; private set; } + public double DurationSeconds { get; private set; } + + public int Added { get; private set; } + public int Merged { get; private set; } + public int Replaced { get; private set; } + public int Skipped { get; private set; } + public int Total => this.Added + this.Merged + this.Replaced + this.Skipped; + + public double AddedPercentage { get; private set; } + public double MergedPercentage { get; private set; } + public double ReplacedPercentage { get; private set; } + public double SkippedPercentage { get; private set; } + + [JsonProperty("addedTiles")] + public List AddedTiles { get; } = new List(); + + public MergeReport(string jobId, string taskId, string taskType, string targetFormat, bool isNewTarget) + { + this.JobId = jobId; + this.TaskId = taskId; + this.TaskType = taskType; + this.TargetFormat = targetFormat; + this.IsNewTarget = isNewTarget; + } + + public void RecordAdded(Coord coord) + { + this.Added++; + this.AddedTiles.Add(coord); + } + + public void RecordMerged() => this.Merged++; + public void RecordReplaced() => this.Replaced++; + public void RecordSkipped() => this.Skipped++; + + public void Finalize(DateTime startTime, DateTime endTime) + { + this.StartTime = startTime; + this.EndTime = endTime; + this.DurationSeconds = (endTime - startTime).TotalSeconds; + + int total = this.Total; + if (total > 0) + { + this.AddedPercentage = 100.0 * this.Added / total; + this.MergedPercentage = 100.0 * this.Merged / total; + this.ReplacedPercentage = 100.0 * this.Replaced / total; + this.SkippedPercentage = 100.0 * this.Skipped / total; + } + } + + public string ToJson() => JsonConvert.SerializeObject(this, Formatting.None); + + // Summary for the structured log line: counts + percentages, WITHOUT the added-tile list. + public string ToLogString() + { + var summary = new + { + version = this.Version, + jobId = this.JobId, + taskId = this.TaskId, + taskType = this.TaskType, + targetFormat = this.TargetFormat, + isNewTarget = this.IsNewTarget, + durationSeconds = this.DurationSeconds, + counts = new { added = this.Added, merged = this.Merged, replaced = this.Replaced, skipped = this.Skipped, total = this.Total }, + percentages = new { added = this.AddedPercentage, merged = this.MergedPercentage, replaced = this.ReplacedPercentage, skipped = this.SkippedPercentage } + }; + return JsonConvert.SerializeObject(summary, Formatting.None); + } + } +} diff --git a/MergerServiceUnitTests/Models/MergeReportTest.cs b/MergerServiceUnitTests/Models/MergeReportTest.cs new file mode 100644 index 0000000..3cbc7a5 --- /dev/null +++ b/MergerServiceUnitTests/Models/MergeReportTest.cs @@ -0,0 +1,58 @@ +using MergerLogic.DataTypes; +using MergerService.Models.Reports; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Newtonsoft.Json.Linq; +using System; + +namespace MergerServiceUnitTests.Models +{ + [TestClass] + [TestCategory("unit")] + public class MergeReportTest + { + [TestMethod] + public void Counts_And_Percentages_Are_Computed() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordAdded(new Coord(10, 1, 2)); + report.RecordAdded(new Coord(10, 1, 3)); + report.RecordMerged(); + report.RecordReplaced(); + report.RecordSkipped(); + + report.Finalize(new DateTime(2026, 9, 15, 0, 0, 0, DateTimeKind.Utc), + new DateTime(2026, 9, 15, 0, 1, 0, DateTimeKind.Utc)); + + Assert.AreEqual(2, report.Added); + Assert.AreEqual(1, report.Merged); + Assert.AreEqual(1, report.Replaced); + Assert.AreEqual(1, report.Skipped); + Assert.AreEqual(5, report.Total); + Assert.AreEqual(60, report.DurationSeconds); + Assert.AreEqual(40.0, report.AddedPercentage, 0.01); + } + + [TestMethod] + public void Json_Includes_AddedTiles_LogString_Excludes_Them() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordAdded(new Coord(10, 1, 2)); + report.Finalize(DateTime.UnixEpoch, DateTime.UnixEpoch); + + JObject json = JObject.Parse(report.ToJson()); + Assert.AreEqual(1, ((JArray)json["addedTiles"]).Count); + + Assert.IsFalse(report.ToLogString().Contains("addedTiles")); + Assert.IsTrue(report.ToLogString().Contains("\"added\"")); + } + + [TestMethod] + public void Percentages_Are_Zero_When_No_Tiles() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", true); + report.Finalize(DateTime.UnixEpoch, DateTime.UnixEpoch); + Assert.AreEqual(0, report.Total); + Assert.AreEqual(0.0, report.AddedPercentage, 0.01); + } + } +} From 571a23b30482a44a8dca2529f29381c1a2e00327 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:44:11 +0300 Subject: [PATCH 08/27] docs: plan Task 3 emits camelCase JSON to match spec artifact shape Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-09-15-tile-merge-report.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/docs/superpowers/plans/2026-09-15-tile-merge-report.md b/docs/superpowers/plans/2026-09-15-tile-merge-report.md index c6df84d..416a541 100644 --- a/docs/superpowers/plans/2026-09-15-tile-merge-report.md +++ b/docs/superpowers/plans/2026-09-15-tile-merge-report.md @@ -407,6 +407,7 @@ Create `MergerService/Models/Reports/MergeReport.cs`: ```csharp using MergerLogic.DataTypes; using Newtonsoft.Json; +using Newtonsoft.Json.Serialization; namespace MergerService.Models.Reports { @@ -471,7 +472,9 @@ namespace MergerService.Models.Reports } } - public string ToJson() => JsonConvert.SerializeObject(this, Formatting.None); + // camelCase so the artifact matches the spec JSON shape (jobId, counts, z/x/y) + public string ToJson() => JsonConvert.SerializeObject(this, Formatting.None, + new JsonSerializerSettings { ContractResolver = new CamelCasePropertyNamesContractResolver() }); // Summary for the structured log line: counts + percentages, WITHOUT the added-tile list. public string ToLogString() @@ -494,13 +497,7 @@ namespace MergerService.Models.Reports } ``` -Note: `ToJson()` serializes `AddedTiles` (property name `addedTiles` via camelCase is NOT applied by default in Newtonsoft — the test parses `json["addedTiles"]`). To match, add `[JsonProperty("addedTiles")]` on `AddedTiles` and `[JsonProperty("added")]` etc. only where the test asserts specific names. Concretely add these attributes: - -```csharp -[JsonProperty("addedTiles")] public List AddedTiles { get; } = new List(); -``` - -And in `ToJson()` the top-level `added` count is asserted in the log-string test only (which uses the anonymous object). For `ToJson` the test only checks `addedTiles`, so the single attribute above is sufficient. +Note: `ToJson()` uses `CamelCasePropertyNamesContractResolver`, so all property/field names serialize camelCase (`jobId`, `addedTiles`, and `Coord` → `z/x/y`), matching the spec's artifact shape. The explicit `[JsonProperty("addedTiles")]` on `AddedTiles` is kept but redundant under the resolver. - [ ] **Step 4: Run to verify pass** From e9a15c6fb746bf5ffa11d5e38b48f461cfd25725 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:45:16 +0300 Subject: [PATCH 09/27] fix: serialize MergeReport JSON as camelCase Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Models/Reports/MergeReport.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/MergerService/Models/Reports/MergeReport.cs b/MergerService/Models/Reports/MergeReport.cs index 7abea1c..4129573 100644 --- a/MergerService/Models/Reports/MergeReport.cs +++ b/MergerService/Models/Reports/MergeReport.cs @@ -1,5 +1,6 @@ using MergerLogic.DataTypes; using Newtonsoft.Json; +using Newtonsoft.Json.Serialization; namespace MergerService.Models.Reports { @@ -65,7 +66,8 @@ public void Finalize(DateTime startTime, DateTime endTime) } } - public string ToJson() => JsonConvert.SerializeObject(this, Formatting.None); + public string ToJson() => JsonConvert.SerializeObject(this, Formatting.None, + new JsonSerializerSettings { ContractResolver = new CamelCasePropertyNamesContractResolver() }); // Summary for the structured log line: counts + percentages, WITHOUT the added-tile list. public string ToLogString() From 9888a915465a28833786065ea90133849829a662 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:45:17 +0300 Subject: [PATCH 10/27] feat: add ReportWriter with FS and S3 sinks for merge report artifact Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Utils/IReportWriter.cs | 11 +++ MergerService/Utils/ReportWriter.cs | 73 ++++++++++++++++ .../MergerServiceUnitTests.csproj | 1 + .../Utils/ReportWriterTest.cs | 84 +++++++++++++++++++ 4 files changed, 169 insertions(+) create mode 100644 MergerService/Utils/IReportWriter.cs create mode 100644 MergerService/Utils/ReportWriter.cs create mode 100644 MergerServiceUnitTests/Utils/ReportWriterTest.cs diff --git a/MergerService/Utils/IReportWriter.cs b/MergerService/Utils/IReportWriter.cs new file mode 100644 index 0000000..31f0f00 --- /dev/null +++ b/MergerService/Utils/IReportWriter.cs @@ -0,0 +1,11 @@ +using MergerService.Models.Reports; + +namespace MergerService.Utils +{ + public interface IReportWriter + { + // Writes the report JSON artifact to the configured sink under outputPath. + // No-op when outputPath is null/empty. Throws on write failure. + void WriteReport(MergeReport report, string? outputPath); + } +} diff --git a/MergerService/Utils/ReportWriter.cs b/MergerService/Utils/ReportWriter.cs new file mode 100644 index 0000000..59bb769 --- /dev/null +++ b/MergerService/Utils/ReportWriter.cs @@ -0,0 +1,73 @@ +using Amazon.S3; +using Amazon.S3.Model; +using MergerLogic.Utils; +using MergerService.Models.Reports; +using Microsoft.Extensions.Logging; +using System.IO.Abstractions; +using System.Reflection; + +namespace MergerService.Utils +{ + public class ReportWriter : IReportWriter + { + private readonly IConfigurationManager _configuration; + private readonly IFileSystem _fileSystem; + private readonly IAmazonS3 _s3; + private readonly ILogger _logger; + + public ReportWriter(IConfigurationManager configuration, IFileSystem fileSystem, IAmazonS3 s3, + ILogger logger) + { + this._configuration = configuration; + this._fileSystem = fileSystem; + this._s3 = s3; + this._logger = logger; + } + + public void WriteReport(MergeReport report, string? outputPath) + { + string methodName = MethodBase.GetCurrentMethod().Name; + if (string.IsNullOrEmpty(outputPath)) + { + this._logger.LogDebug($"[{methodName}] No ReportOutputPath configured, skipping report artifact"); + return; + } + + string fileName = $"merge-report-{report.JobId}-{report.TaskId}.json"; + string json = report.ToJson(); + string sink = this._configuration.GetConfiguration("REPORT", "sink"); + + if (string.Equals(sink, "S3", System.StringComparison.OrdinalIgnoreCase)) + { + this.WriteToS3(outputPath, fileName, json); + } + else + { + this.WriteToFs(outputPath, fileName, json); + } + + this._logger.LogInformation($"[{methodName}] Wrote merge report to {sink}:{outputPath}/{fileName}"); + } + + private void WriteToFs(string outputPath, string fileName, string json) + { + this._fileSystem.Directory.CreateDirectory(outputPath); + string fullPath = this._fileSystem.Path.Combine(outputPath, fileName); + this._fileSystem.File.WriteAllText(fullPath, json); + } + + private void WriteToS3(string outputPath, string fileName, string json) + { + string bucket = this._configuration.GetConfiguration("S3", "bucket"); + string key = $"{outputPath.TrimEnd('/')}/{fileName}"; + var request = new PutObjectRequest + { + BucketName = bucket, + Key = key, + ContentBody = json, + ContentType = "application/json" + }; + var res = this._s3.PutObjectAsync(request).Result; + } + } +} diff --git a/MergerServiceUnitTests/MergerServiceUnitTests.csproj b/MergerServiceUnitTests/MergerServiceUnitTests.csproj index 4213e99..7742667 100644 --- a/MergerServiceUnitTests/MergerServiceUnitTests.csproj +++ b/MergerServiceUnitTests/MergerServiceUnitTests.csproj @@ -12,6 +12,7 @@ + all runtime; build; native; contentfiles; analyzers; buildtransitive diff --git a/MergerServiceUnitTests/Utils/ReportWriterTest.cs b/MergerServiceUnitTests/Utils/ReportWriterTest.cs new file mode 100644 index 0000000..378eed4 --- /dev/null +++ b/MergerServiceUnitTests/Utils/ReportWriterTest.cs @@ -0,0 +1,84 @@ +using Amazon.S3; +using Amazon.S3.Model; +using MergerLogic.Utils; +using MergerService.Models.Reports; +using MergerService.Utils; +using Microsoft.Extensions.Logging; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using System.IO.Abstractions; +using System.IO.Abstractions.TestingHelpers; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +namespace MergerServiceUnitTests.Utils +{ + [TestClass] + [TestCategory("unit")] + public class ReportWriterTest + { + private Mock _config; + private Mock> _logger; + private Mock _s3; + + [TestInitialize] + public void BeforeEach() + { + this._config = new Mock(MockBehavior.Loose); + this._logger = new Mock>(MockBehavior.Loose); + this._s3 = new Mock(MockBehavior.Loose); + } + + private MergeReport BuildReport() + { + var r = new MergeReport("job1", "task1", "MERGE", "PNG", false); + r.Finalize(System.DateTime.UnixEpoch, System.DateTime.UnixEpoch); + return r; + } + + [TestMethod] + public void FsSink_WritesJsonFile() + { + this._config.Setup(c => c.GetConfiguration("REPORT", "sink")).Returns("FS"); + var fs = new MockFileSystem(); + var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + + writer.WriteReport(this.BuildReport(), "/reports"); + + string expected = fs.Path.Combine("/reports", "merge-report-job1-task1.json"); + Assert.IsTrue(fs.FileExists(expected)); + StringAssert.Contains(fs.File.ReadAllText(expected), "\"jobId\":\"job1\""); + } + + [TestMethod] + public void S3Sink_PutsObject() + { + this._config.Setup(c => c.GetConfiguration("REPORT", "sink")).Returns("S3"); + this._config.Setup(c => c.GetConfiguration("S3", "bucket")).Returns("tiles"); + this._s3.Setup(s => s.PutObjectAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(new PutObjectResponse()); + var fs = new MockFileSystem(); + var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + + writer.WriteReport(this.BuildReport(), "reports"); + + this._s3.Verify(s => s.PutObjectAsync( + It.Is(r => r.BucketName == "tiles" && r.Key == "reports/merge-report-job1-task1.json"), + It.IsAny()), Times.Once); + } + + [TestMethod] + public void EmptyPath_IsNoOp() + { + var fs = new MockFileSystem(); + var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + + writer.WriteReport(this.BuildReport(), null); + writer.WriteReport(this.BuildReport(), ""); + + Assert.AreEqual(0, fs.AllFiles.Count()); + this._s3.Verify(s => s.PutObjectAsync(It.IsAny(), It.IsAny()), Times.Never); + } + } +} From ad5db1f61efa9748e6a44bb99a83554992bcf350 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:46:42 +0300 Subject: [PATCH 11/27] feat: add ReportOutputPath to job AdditionalParams Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Models/Jobs/JobParamersAdditiomalParams.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/MergerService/Models/Jobs/JobParamersAdditiomalParams.cs b/MergerService/Models/Jobs/JobParamersAdditiomalParams.cs index 9ef7b23..f212670 100644 --- a/MergerService/Models/Jobs/JobParamersAdditiomalParams.cs +++ b/MergerService/Models/Jobs/JobParamersAdditiomalParams.cs @@ -7,13 +7,15 @@ namespace MergerService.Models.Jobs public class AdditionalParams { [JsonInclude] public string? JobTrackerServiceURL { get; } + [JsonInclude] public string? ReportOutputPath { get; } [System.Text.Json.Serialization.JsonIgnore] private JsonSerializerSettings _jsonSerializerSettings; - public AdditionalParams(string jobTrackerServiceURL) + public AdditionalParams(string jobTrackerServiceURL, string? reportOutputPath = null) { this.JobTrackerServiceURL = jobTrackerServiceURL; + this.ReportOutputPath = reportOutputPath; this._jsonSerializerSettings = new JsonSerializerSettings(); this._jsonSerializerSettings.Converters.Add(new StringEnumConverter()); From ae09d512c14a90b27a6a3c75ad7f2ce1b402872b Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 15:56:09 +0300 Subject: [PATCH 12/27] feat: classify added/merged/replaced tiles and emit merge report in TaskExecutor Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Runners/ITaskExecutor.cs | 2 +- MergerService/Runners/TaskExecutor.cs | 52 ++++- MergerService/Runners/TaskRunner.cs | 2 +- .../Runners/TaskExecutorTest.cs | 177 +++++++++++++++++- .../Runners/TaskRunnerTest.cs | 4 +- 5 files changed, 226 insertions(+), 11 deletions(-) diff --git a/MergerService/Runners/ITaskExecutor.cs b/MergerService/Runners/ITaskExecutor.cs index 1a2cdd8..503995c 100644 --- a/MergerService/Runners/ITaskExecutor.cs +++ b/MergerService/Runners/ITaskExecutor.cs @@ -5,6 +5,6 @@ namespace MergerService.Runners { public interface ITaskExecutor { - void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl); + void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl, string? reportOutputPath); } } diff --git a/MergerService/Runners/TaskExecutor.cs b/MergerService/Runners/TaskExecutor.cs index b63b1a3..12e5911 100644 --- a/MergerService/Runners/TaskExecutor.cs +++ b/MergerService/Runners/TaskExecutor.cs @@ -4,6 +4,7 @@ using MergerLogic.Monitoring.Metrics; using MergerLogic.Utils; using MergerService.Controllers; +using MergerService.Models.Reports; using MergerService.Models.Tasks; using MergerService.Utils; using System.Diagnostics; @@ -21,6 +22,7 @@ public class TaskExecutor : ITaskExecutor private readonly ActivitySource _activitySource; private readonly IFileSystem _fileSystem; private readonly IMetricsProvider _metricsProvider; + private readonly IReportWriter _reportWriter; private readonly string _inputPath; private readonly string _gpkgPath; private readonly bool _limitBatchSize; @@ -32,7 +34,7 @@ public class TaskExecutor : ITaskExecutor public TaskExecutor(IDataFactory dataFactory, ITileMerger tileMerger, ITimeUtils timeUtils, IConfigurationManager configurationManager, ILogger logger, ActivitySource activitySource, - IFileSystem fileSystem, IMetricsProvider metricsProvider) + IFileSystem fileSystem, IMetricsProvider metricsProvider, IReportWriter reportWriter) { this._dataFactory = dataFactory; this._tileMerger = tileMerger; @@ -41,6 +43,7 @@ public TaskExecutor(IDataFactory dataFactory, ITileMerger tileMerger, ITimeUtils this._activitySource = activitySource; this._fileSystem = fileSystem; this._metricsProvider = metricsProvider; + this._reportWriter = reportWriter; this._inputPath = configurationManager.GetConfiguration("GENERAL", "inputPath"); this._gpkgPath = configurationManager.GetConfiguration("GENERAL", "gpkgPath"); this._filePath = configurationManager.GetConfiguration("GENERAL", "filePath"); @@ -61,7 +64,7 @@ public TaskExecutor(IDataFactory dataFactory, ITileMerger tileMerger, ITimeUtils } } - public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl) + public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl, string? reportOutputPath) { string methodName = MethodBase.GetCurrentMethod().Name; this._logger.LogDebug($"[{methodName}] start {task.ToString()}"); @@ -83,6 +86,9 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal } MergeMetadata metadata = task.Parameters; + DateTime reportStart = DateTime.UtcNow; + MergeReport report = new MergeReport(task.JobId, task.Id, task.Type, + metadata.TargetFormat.ToString(), metadata.IsNewTarget); Stopwatch mergeRunTimeStopwatch = new Stopwatch(); TimeSpan ts; @@ -162,13 +168,35 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal // TODO: upscale = false - this is a temporary fix till we decide how sources should be upscaled correspondingTileBuilders.Add(() => source.GetCorrespondingTile(coord, false)); } + // TileExists mutates coord.Y in place; pass a copy so the coord the + // merge builders capture stays in its original grid/origin space. + bool existedBefore = !metadata.IsNewTarget && target.TileExists(new Coord(coord.Z, coord.X, coord.Y)); + var tileMergeStopwatch = Stopwatch.StartNew(); - Tile? tile = this._tileMerger.MergeTiles(correspondingTileBuilders, coord, strategy, metadata.IsNewTarget); + Tile? tile = this._tileMerger.MergeTiles(correspondingTileBuilders, coord, strategy, out MergeStats stats, metadata.IsNewTarget); tileMergeStopwatch.Stop(); this._metricsProvider.MergeTimePerTileHistogram(tileMergeStopwatch.Elapsed.TotalSeconds, metadata.TargetFormat); if (tile != null) { + if (!stats.AnySourceUsed) + { + // target re-encode with no source data — not a real change + report.RecordSkipped(); + } + else if (!existedBefore) + { + report.RecordAdded(new Coord(coord.Z, coord.X, coord.Y)); + } + else if (stats.TargetUsed) + { + report.RecordMerged(); + } + else + { + report.RecordReplaced(); + } + tiles.Add(tile); currentBatchBytes += tile.Size(); @@ -182,6 +210,10 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal currentBatchBytes = 0; } } + else + { + report.RecordSkipped(); + } tileProgressCount++; overallTileProgressCount++; @@ -253,6 +285,20 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal } target.Wrapup(); } + + report.Finalize(reportStart, DateTime.UtcNow); + this._logger.LogInformation($"[{methodName}] Merge report: {report.ToLogString()}"); + try + { + this._reportWriter.WriteReport(report, reportOutputPath); + } + catch (Exception e) + { + // Best-effort (proposed default). Whether this should fail the task is an open + // question raised on the implementation PR. + this._logger.LogError(e, $"[{methodName}] Failed to write merge report artifact: {e.Message}"); + } + this._logger.LogDebug($"[{methodName}] end"); } diff --git a/MergerService/Runners/TaskRunner.cs b/MergerService/Runners/TaskRunner.cs index 0088416..57d716b 100644 --- a/MergerService/Runners/TaskRunner.cs +++ b/MergerService/Runners/TaskRunner.cs @@ -114,7 +114,7 @@ public bool RunTask(MergeTask? task) try { this._heartbeatClient.Start(task.Id); - this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl); + this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl, null); taskSucceed = true; } catch (Exception e) diff --git a/MergerServiceUnitTests/Runners/TaskExecutorTest.cs b/MergerServiceUnitTests/Runners/TaskExecutorTest.cs index 5522ac5..cf3807d 100644 --- a/MergerServiceUnitTests/Runners/TaskExecutorTest.cs +++ b/MergerServiceUnitTests/Runners/TaskExecutorTest.cs @@ -4,6 +4,7 @@ using MergerLogic.Monitoring.Metrics; using MergerLogic.Utils; using MergerService.Controllers; +using MergerService.Models.Reports; using MergerService.Models.Tasks; using MergerService.Runners; using MergerService.Utils; @@ -37,6 +38,8 @@ public class TaskExecutorTest private Mock _taskUtilsMock; private Mock _tileScalerMock; private Mock> _tileMergerLoggerMock; + private Mock _reportWriterMock; + private Mock _tileMergerMock; private ActivitySource _testActivitySource; private ITileMerger _testTileMerger; @@ -67,6 +70,8 @@ public void BeforeEach() this._taskUtilsMock = this._mockRepository.Create(); this._tileScalerMock = this._mockRepository.Create(); this._tileMergerLoggerMock = this._mockRepository.Create>(); + this._reportWriterMock = this._mockRepository.Create(); + this._tileMergerMock = this._mockRepository.Create(); this._testActivitySource = new ActivitySource("test"); this._testTileMerger = new TileMerger(_tileScalerMock.Object, _tileMergerLoggerMock.Object); @@ -89,13 +94,13 @@ public void WhenGivenSourcesWithOneTile_ShouldWriteAllTilesToTarget(int numberOf var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _testTileMerger, _timeUtilsMock.Object, _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, - _metricsProviderMock.Object); + _metricsProviderMock.Object, _reportWriterMock.Object); targetDataMock.Setup(targetData => targetData.UpdateTiles(It.IsAny>())).Callback>( resultWrittenTiles.AddRange ); - testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null); + testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, null); targetDataMock.Verify(targetData => targetData.UpdateTiles(It.Is>( tiles => tiles.All( @@ -174,13 +179,13 @@ public void WhenConfiguringBatchLimits_ShouldWriteTilesEachTimeAfterReachingBatc var resultWrittenTiles = new List(); var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _testTileMerger, _timeUtilsMock.Object, _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, - _metricsProviderMock.Object); + _metricsProviderMock.Object, _reportWriterMock.Object); targetDataMock.Setup(targetData => targetData.UpdateTiles(It.IsAny>())).Callback>( resultWrittenTiles.AddRange ); - testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null); + testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, null); targetDataMock.Verify(targetData => targetData.UpdateTiles(It.IsAny>()), Times.Exactly(amountOfFlushes)); targetDataMock.Verify(targetData => targetData.Wrapup(), Times.Once); @@ -190,6 +195,170 @@ public void WhenConfiguringBatchLimits_ShouldWriteTilesEachTimeAfterReachingBatc )); } + [TestMethod] + [TestCategory("unit")] + [TestCategory("runners")] + public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() + { + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); + + byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); + Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); + Source testSource = new Source("source", "source_type"); + Mock targetDataMock = this._mockRepository.Create(); + Mock sourceDataMock = this._mockRepository.Create(); + + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testTarget.Type, testTarget.Path, It.IsAny(), + testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) + ).Returns(targetDataMock.Object); + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testSource.Type, testSource.Path, It.IsAny(), + testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) + ).Returns(sourceDataMock.Object); + + targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(true); + + TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); + var testTask = new MergeTask("id", "type", "description", + new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), + Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); + + MergeStats outStats = new MergeStats(true, true); + this._tileMergerMock.Setup(m => m.MergeTiles( + It.IsAny>(), It.IsAny(), + It.IsAny(), out outStats, It.IsAny()) + ).Returns(new Tile(new Coord(1, 1, 1), tileBytes)); + + MergeReport captured = null; + this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) + .Callback((r, p) => captured = r); + + var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, + _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, + _metricsProviderMock.Object, _reportWriterMock.Object); + + testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); + + Assert.IsNotNull(captured); + Assert.AreEqual(1, captured.Merged); + Assert.AreEqual(0, captured.Added); + Assert.AreEqual(0, captured.Replaced); + Assert.AreEqual(0, captured.Skipped); + } + + [TestMethod] + [TestCategory("unit")] + [TestCategory("runners")] + public void ExecuteTask_NewTargetTile_CountsAsAdded() + { + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); + + byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); + Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); + Source testSource = new Source("source", "source_type"); + Mock targetDataMock = this._mockRepository.Create(); + Mock sourceDataMock = this._mockRepository.Create(); + + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testTarget.Type, testTarget.Path, It.IsAny(), + testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) + ).Returns(targetDataMock.Object); + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testSource.Type, testSource.Path, It.IsAny(), + testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) + ).Returns(sourceDataMock.Object); + + targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(false); + + TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); + var testTask = new MergeTask("id", "type", "description", + new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), + Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); + + MergeStats outStats = new MergeStats(false, true); + this._tileMergerMock.Setup(m => m.MergeTiles( + It.IsAny>(), It.IsAny(), + It.IsAny(), out outStats, It.IsAny()) + ).Returns(new Tile(new Coord(1, 1, 1), tileBytes)); + + MergeReport captured = null; + this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) + .Callback((r, p) => captured = r); + + var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, + _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, + _metricsProviderMock.Object, _reportWriterMock.Object); + + testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); + + Assert.IsNotNull(captured); + Assert.AreEqual(1, captured.Added); + Assert.AreEqual(1, captured.AddedTiles.Count); + Assert.AreEqual(1, captured.AddedTiles[0].Z); + Assert.AreEqual(1, captured.AddedTiles[0].X); + Assert.AreEqual(1, captured.AddedTiles[0].Y); + Assert.AreEqual(0, captured.Merged); + Assert.AreEqual(0, captured.Replaced); + } + + [TestMethod] + [TestCategory("unit")] + [TestCategory("runners")] + public void ExecuteTask_OpaqueSourceOverExisting_CountsAsReplaced() + { + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); + + byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); + Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); + Source testSource = new Source("source", "source_type"); + Mock targetDataMock = this._mockRepository.Create(); + Mock sourceDataMock = this._mockRepository.Create(); + + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testTarget.Type, testTarget.Path, It.IsAny(), + testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) + ).Returns(targetDataMock.Object); + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testSource.Type, testSource.Path, It.IsAny(), + testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) + ).Returns(sourceDataMock.Object); + + targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(true); + + TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); + var testTask = new MergeTask("id", "type", "description", + new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), + Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); + + MergeStats outStats = new MergeStats(false, true); + this._tileMergerMock.Setup(m => m.MergeTiles( + It.IsAny>(), It.IsAny(), + It.IsAny(), out outStats, It.IsAny()) + ).Returns(new Tile(new Coord(1, 1, 1), tileBytes)); + + MergeReport captured = null; + this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) + .Callback((r, p) => captured = r); + + var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, + _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, + _metricsProviderMock.Object, _reportWriterMock.Object); + + testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); + + Assert.IsNotNull(captured); + Assert.AreEqual(1, captured.Replaced); + Assert.AreEqual(0, captured.Added); + Assert.AreEqual(0, captured.Merged); + } + private Tuple, Tile[]> SetupTestTask(int amountOfSources, bool isTargetNew) { byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); diff --git a/MergerServiceUnitTests/Runners/TaskRunnerTest.cs b/MergerServiceUnitTests/Runners/TaskRunnerTest.cs index c518ee4..36e9470 100644 --- a/MergerServiceUnitTests/Runners/TaskRunnerTest.cs +++ b/MergerServiceUnitTests/Runners/TaskRunnerTest.cs @@ -73,7 +73,7 @@ public void WhenTaskExecutedSuccessfully_ShouldUpdateTaskCompletion() Status.PENDING, 0, "reason", 0, "testJobId", true, new DateTime(), new DateTime()); this._taskUtilsMock.Setup(taskUtils => taskUtils.GetTask(It.IsAny(), It.IsAny())).Returns(testTask); - this._taskExecutorMock.Setup(taskExecutor => taskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, It.IsAny())); + this._taskExecutorMock.Setup(taskExecutor => taskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, It.IsAny(), It.IsAny())); var testTaskRunner = new TaskRunner(_taskExecutorMock.Object, _jobUtilsMock.Object, _loggerMock.Object, _taskUtilsMock.Object, _heartbeatClientMock.Object, _metricsProviderMock.Object, @@ -95,7 +95,7 @@ public void WhenTaskExecutionFailed_ShouldUpdateTaskFailed() Status.PENDING, 0, "reason", 0, "testJobId", true, new DateTime(), new DateTime()); this._taskUtilsMock.Setup(taskUtils => taskUtils.GetTask(It.IsAny(), It.IsAny())).Returns(testTask); - this._taskExecutorMock.Setup(taskExecutor => taskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, It.IsAny())).Throws(new Exception(testFailureMessage)); + this._taskExecutorMock.Setup(taskExecutor => taskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, It.IsAny(), It.IsAny())).Throws(new Exception(testFailureMessage)); var testTaskRunner = new TaskRunner(_taskExecutorMock.Object, _jobUtilsMock.Object, _loggerMock.Object, _taskUtilsMock.Object, _heartbeatClientMock.Object, _metricsProviderMock.Object, From 79c9154dbb753cced446c2416611edd413e285c9 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 16:03:27 +0300 Subject: [PATCH 13/27] feat: pass job ReportOutputPath from TaskRunner to TaskExecutor Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Runners/TaskRunner.cs | 7 +++++-- MergerServiceUnitTests/Runners/TaskRunnerTest.cs | 8 ++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/MergerService/Runners/TaskRunner.cs b/MergerService/Runners/TaskRunner.cs index 57d716b..6271516 100644 --- a/MergerService/Runners/TaskRunner.cs +++ b/MergerService/Runners/TaskRunner.cs @@ -1,5 +1,6 @@ using MergerLogic.Clients; using MergerLogic.Monitoring.Metrics; +using MergerService.Models.Jobs; using MergerService.Models.Tasks; using MergerService.Utils; using System.Diagnostics; @@ -87,7 +88,9 @@ public bool RunTask(MergeTask? task) } this._logger.LogInformation($"[{methodName}] Run Task: jobId {task.JobId}, taskId {task.Id}"); - string? managerCallbackUrl = this._jobUtils.GetJob(task.JobId)?.Parameters.AdditionalParams?.JobTrackerServiceURL; + MergeJob? job = this._jobUtils.GetJob(task.JobId); + string? managerCallbackUrl = job?.Parameters.AdditionalParams?.JobTrackerServiceURL; + string? reportOutputPath = job?.Parameters.AdditionalParams?.ReportOutputPath; string log = managerCallbackUrl == null ? "managerCallbackUrl not provided as job parameter" : $"managerCallback url: {managerCallbackUrl}"; this._logger.LogDebug($"[{methodName}]{log}"); @@ -114,7 +117,7 @@ public bool RunTask(MergeTask? task) try { this._heartbeatClient.Start(task.Id); - this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl, null); + this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl, reportOutputPath); taskSucceed = true; } catch (Exception e) diff --git a/MergerServiceUnitTests/Runners/TaskRunnerTest.cs b/MergerServiceUnitTests/Runners/TaskRunnerTest.cs index 36e9470..c145c44 100644 --- a/MergerServiceUnitTests/Runners/TaskRunnerTest.cs +++ b/MergerServiceUnitTests/Runners/TaskRunnerTest.cs @@ -3,6 +3,7 @@ using MergerLogic.ImageProcessing; using MergerLogic.Monitoring.Metrics; using MergerService.Controllers; +using MergerService.Models.Jobs; using MergerService.Models.Tasks; using MergerService.Runners; using MergerService.Utils; @@ -72,7 +73,13 @@ public void WhenTaskExecutedSuccessfully_ShouldUpdateTaskCompletion() new MergeMetadata(TileFormat.Jpeg, true, new TileBounds[0], new Source[0]), Status.PENDING, 0, "reason", 0, "testJobId", true, new DateTime(), new DateTime()); + var testJob = new MergeJob("testJobId", "resourceId", "version", "type", "resolution", "description", + new JobMergeMetadata(null!, new string[0], "", "", new AdditionalParams("http://tracker", "reports")), + new DateTime(), new DateTime(), Status.PENDING, 0, "reason", false, 0, "internalId", "producerName", + "productName", "productType", 0, 0, 0, 0, 0, 0, 0, "additionalIdentifiers", "domain", new MergeTask[0]); + this._taskUtilsMock.Setup(taskUtils => taskUtils.GetTask(It.IsAny(), It.IsAny())).Returns(testTask); + this._jobUtilsMock.Setup(jobUtils => jobUtils.GetJob(testTask.JobId)).Returns(testJob); this._taskExecutorMock.Setup(taskExecutor => taskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, It.IsAny(), It.IsAny())); var testTaskRunner = new TaskRunner(_taskExecutorMock.Object, _jobUtilsMock.Object, _loggerMock.Object, @@ -83,6 +90,7 @@ public void WhenTaskExecutedSuccessfully_ShouldUpdateTaskCompletion() testTaskRunner.RunTask(testResultTask); Assert.AreEqual(testTask, testResultTask); + this._taskExecutorMock.Verify(e => e.ExecuteTask(testTask, _taskUtilsMock.Object, It.IsAny(), "reports"), Times.Once); _taskUtilsMock.Verify(taskUtils => taskUtils.UpdateCompletion(testTask.JobId, testTask.Id, It.IsAny()), Times.Once); } From 7f5e229f1b378bf737baec6edc646e669ed37b15 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 16:05:02 +0300 Subject: [PATCH 14/27] build: register ReportWriter and add REPORT sink config Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Program.cs | 1 + MergerService/appsettings.json | 3 +++ 2 files changed, 4 insertions(+) diff --git a/MergerService/Program.cs b/MergerService/Program.cs index e327200..7379ecb 100644 --- a/MergerService/Program.cs +++ b/MergerService/Program.cs @@ -29,6 +29,7 @@ builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); +builder.Services.AddSingleton(); builder.Services.AddSingleton(); var app = builder.Build(); diff --git a/MergerService/appsettings.json b/MergerService/appsettings.json index 1d96c4c..2192956 100644 --- a/MergerService/appsettings.json +++ b/MergerService/appsettings.json @@ -52,6 +52,9 @@ "port": 9500, "measurementBuckets": "[0.001, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 15, 50,250, 500]" }, + "REPORT": { + "sink": "FS" + }, "AllowedHosts": "*", "HTTP": { "retries": 3 From 148e403e8d5b4af405a1dfc1fa1cbc9358032272 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 16:19:19 +0300 Subject: [PATCH 15/27] fix: resolve S3 client lazily in ReportWriter and add skipped-path tests Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Runners/TaskExecutor.cs | 2 +- MergerService/Utils/ReportWriter.cs | 10 +- .../Runners/TaskExecutorTest.cs | 107 ++++++++++++++++++ .../Utils/ReportWriterTest.cs | 11 +- 4 files changed, 122 insertions(+), 8 deletions(-) diff --git a/MergerService/Runners/TaskExecutor.cs b/MergerService/Runners/TaskExecutor.cs index 12e5911..8b73692 100644 --- a/MergerService/Runners/TaskExecutor.cs +++ b/MergerService/Runners/TaskExecutor.cs @@ -181,7 +181,7 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal { if (!stats.AnySourceUsed) { - // target re-encode with no source data — not a real change + // No source contributed data → not counted as a real change (skipped), even if the target coord was empty before. report.RecordSkipped(); } else if (!existedBefore) diff --git a/MergerService/Utils/ReportWriter.cs b/MergerService/Utils/ReportWriter.cs index 59bb769..7de2af9 100644 --- a/MergerService/Utils/ReportWriter.cs +++ b/MergerService/Utils/ReportWriter.cs @@ -2,6 +2,7 @@ using Amazon.S3.Model; using MergerLogic.Utils; using MergerService.Models.Reports; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using System.IO.Abstractions; using System.Reflection; @@ -12,15 +13,15 @@ public class ReportWriter : IReportWriter { private readonly IConfigurationManager _configuration; private readonly IFileSystem _fileSystem; - private readonly IAmazonS3 _s3; + private readonly IServiceProvider _serviceProvider; private readonly ILogger _logger; - public ReportWriter(IConfigurationManager configuration, IFileSystem fileSystem, IAmazonS3 s3, + public ReportWriter(IConfigurationManager configuration, IFileSystem fileSystem, IServiceProvider serviceProvider, ILogger logger) { this._configuration = configuration; this._fileSystem = fileSystem; - this._s3 = s3; + this._serviceProvider = serviceProvider; this._logger = logger; } @@ -67,7 +68,8 @@ private void WriteToS3(string outputPath, string fileName, string json) ContentBody = json, ContentType = "application/json" }; - var res = this._s3.PutObjectAsync(request).Result; + var s3 = this._serviceProvider.GetRequiredService(); + s3.PutObjectAsync(request).Wait(); } } } diff --git a/MergerServiceUnitTests/Runners/TaskExecutorTest.cs b/MergerServiceUnitTests/Runners/TaskExecutorTest.cs index cf3807d..04b78f0 100644 --- a/MergerServiceUnitTests/Runners/TaskExecutorTest.cs +++ b/MergerServiceUnitTests/Runners/TaskExecutorTest.cs @@ -359,6 +359,113 @@ public void ExecuteTask_OpaqueSourceOverExisting_CountsAsReplaced() Assert.AreEqual(0, captured.Merged); } + [TestMethod] + [TestCategory("unit")] + [TestCategory("runners")] + public void ExecuteTask_MergeReturnsNull_CountsAsSkipped() + { + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); + + Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); + Source testSource = new Source("source", "source_type"); + Mock targetDataMock = this._mockRepository.Create(); + Mock sourceDataMock = this._mockRepository.Create(); + + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testTarget.Type, testTarget.Path, It.IsAny(), + testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) + ).Returns(targetDataMock.Object); + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testSource.Type, testSource.Path, It.IsAny(), + testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) + ).Returns(sourceDataMock.Object); + + targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(false); + + TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); + var testTask = new MergeTask("id", "type", "description", + new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), + Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); + + MergeStats outStats = new MergeStats(false, false); + this._tileMergerMock.Setup(m => m.MergeTiles( + It.IsAny>(), It.IsAny(), + It.IsAny(), out outStats, It.IsAny()) + ).Returns((Tile)null); + + MergeReport captured = null; + this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) + .Callback((r, p) => captured = r); + + var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, + _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, + _metricsProviderMock.Object, _reportWriterMock.Object); + + testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); + + Assert.IsNotNull(captured); + Assert.AreEqual(1, captured.Skipped); + Assert.AreEqual(0, captured.Added); + Assert.AreEqual(0, captured.Merged); + Assert.AreEqual(0, captured.Replaced); + } + + [TestMethod] + [TestCategory("unit")] + [TestCategory("runners")] + public void ExecuteTask_TileWithNoSourceData_CountsAsSkipped() + { + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); + this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); + + byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); + Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); + Source testSource = new Source("source", "source_type"); + Mock targetDataMock = this._mockRepository.Create(); + Mock sourceDataMock = this._mockRepository.Create(); + + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testTarget.Type, testTarget.Path, It.IsAny(), + testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) + ).Returns(targetDataMock.Object); + this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( + testSource.Type, testSource.Path, It.IsAny(), + testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) + ).Returns(sourceDataMock.Object); + + targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(false); + + TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); + var testTask = new MergeTask("id", "type", "description", + new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), + Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); + + MergeStats outStats = new MergeStats(targetUsed: true, anySourceUsed: false); + this._tileMergerMock.Setup(m => m.MergeTiles( + It.IsAny>(), It.IsAny(), + It.IsAny(), out outStats, It.IsAny()) + ).Returns(new Tile(new Coord(1, 1, 1), tileBytes)); + + MergeReport captured = null; + this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) + .Callback((r, p) => captured = r); + + var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, + _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, + _metricsProviderMock.Object, _reportWriterMock.Object); + + testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); + + Assert.IsNotNull(captured); + Assert.AreEqual(1, captured.Skipped); + Assert.AreEqual(0, captured.Added); + Assert.AreEqual(0, captured.Merged); + Assert.AreEqual(0, captured.Replaced); + } + private Tuple, Tile[]> SetupTestTask(int amountOfSources, bool isTargetNew) { byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); diff --git a/MergerServiceUnitTests/Utils/ReportWriterTest.cs b/MergerServiceUnitTests/Utils/ReportWriterTest.cs index 378eed4..64ef4e5 100644 --- a/MergerServiceUnitTests/Utils/ReportWriterTest.cs +++ b/MergerServiceUnitTests/Utils/ReportWriterTest.cs @@ -3,9 +3,11 @@ using MergerLogic.Utils; using MergerService.Models.Reports; using MergerService.Utils; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; +using System; using System.IO.Abstractions; using System.IO.Abstractions.TestingHelpers; using System.Linq; @@ -21,6 +23,7 @@ public class ReportWriterTest private Mock _config; private Mock> _logger; private Mock _s3; + private Mock _serviceProvider; [TestInitialize] public void BeforeEach() @@ -28,6 +31,8 @@ public void BeforeEach() this._config = new Mock(MockBehavior.Loose); this._logger = new Mock>(MockBehavior.Loose); this._s3 = new Mock(MockBehavior.Loose); + this._serviceProvider = new Mock(MockBehavior.Loose); + this._serviceProvider.Setup(sp => sp.GetService(typeof(IAmazonS3))).Returns(this._s3.Object); } private MergeReport BuildReport() @@ -42,7 +47,7 @@ public void FsSink_WritesJsonFile() { this._config.Setup(c => c.GetConfiguration("REPORT", "sink")).Returns("FS"); var fs = new MockFileSystem(); - var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + var writer = new ReportWriter(this._config.Object, fs, this._serviceProvider.Object, this._logger.Object); writer.WriteReport(this.BuildReport(), "/reports"); @@ -59,7 +64,7 @@ public void S3Sink_PutsObject() this._s3.Setup(s => s.PutObjectAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(new PutObjectResponse()); var fs = new MockFileSystem(); - var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + var writer = new ReportWriter(this._config.Object, fs, this._serviceProvider.Object, this._logger.Object); writer.WriteReport(this.BuildReport(), "reports"); @@ -72,7 +77,7 @@ public void S3Sink_PutsObject() public void EmptyPath_IsNoOp() { var fs = new MockFileSystem(); - var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); + var writer = new ReportWriter(this._config.Object, fs, this._serviceProvider.Object, this._logger.Object); writer.WriteReport(this.BuildReport(), null); writer.WriteReport(this.BuildReport(), ""); From 9e1d56cbd4507a7e2257efa9da1558bddc95e89e Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 17:43:02 +0300 Subject: [PATCH 16/27] chore: drop superpowers workflow docs from PR Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-09-15-tile-merge-report.md | 1111 ----------------- .../2026-09-15-tile-merge-report-design.md | 157 --- 2 files changed, 1268 deletions(-) delete mode 100644 docs/superpowers/plans/2026-09-15-tile-merge-report.md delete mode 100644 docs/superpowers/specs/2026-09-15-tile-merge-report-design.md diff --git a/docs/superpowers/plans/2026-09-15-tile-merge-report.md b/docs/superpowers/plans/2026-09-15-tile-merge-report.md deleted file mode 100644 index 416a541..0000000 --- a/docs/superpowers/plans/2026-09-15-tile-merge-report.md +++ /dev/null @@ -1,1111 +0,0 @@ -# Tile Merge Report Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Produce a per-task report of tiles added / merged / replaced during a merge, emitted as a structured log line (counts + percentages) and a JSON artifact file (full detail incl. exact added `z/x/y`). - -**Architecture:** `TileMerger` reports whether the target and/or any source contributed to each merged tile via a new `MergeStats` out-param. `TaskExecutor` combines that with an exact-coord `TileExists` pre-check to classify every tile, accumulates counts + the added-tile list into a `MergeReport`, then writes a JSON artifact through `IReportWriter` (FS or S3 by config) and logs a summary. Report destination path comes from a new `AdditionalParams.ReportOutputPath` job field. - -**Tech Stack:** C#/.NET, MSTest + Moq (loose `MockRepository`), Newtonsoft.Json, `System.IO.Abstractions` (`IFileSystem`), AWS SDK (`IAmazonS3`). - -**Spec:** `docs/superpowers/specs/2026-09-15-tile-merge-report-design.md` - ---- - -## File Structure - -- Create `MergerLogic/ImageProcessing/MergeStats.cs` — struct carrying `TargetUsed`, `AnySourceUsed`. -- Modify `MergerLogic/ImageProcessing/ITileMerger.cs` — add `MergeTiles(..., out MergeStats stats)` overload. -- Modify `MergerLogic/ImageProcessing/TileMerger.cs` — populate stats; keep old method as wrapper. -- Create `MergerService/Models/Reports/MergeReport.cs` — accumulator + finalize + serialization. -- Create `MergerService/Utils/IReportWriter.cs` + `MergerService/Utils/ReportWriter.cs` — FS/S3 sink writer. -- Modify `MergerService/Models/Jobs/JobParamersAdditiomalParams.cs` — add `ReportOutputPath`. -- Modify `MergerService/Runners/ITaskExecutor.cs` + `TaskExecutor.cs` — classification, accumulation, emit. -- Modify `MergerService/Runners/TaskRunner.cs` — extract `ReportOutputPath`, pass through. -- Modify `MergerService/Program.cs` — register `IReportWriter`. -- Modify `MergerService/appsettings.json` — add `REPORT` config section. -- Tests: `MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs`, `MergerServiceUnitTests/Models/MergeReportTest.cs`, `MergerServiceUnitTests/Utils/ReportWriterTest.cs`, `MergerServiceUnitTests/Runners/TaskExecutorTest.cs`. - -**Global commands** -- Build: `dotnet build GpkgMerger.sln` -- Test one class: `dotnet test --filter "ClassName~"` - ---- - -## Task 1: `MergeStats` struct - -**Files:** -- Create: `MergerLogic/ImageProcessing/MergeStats.cs` - -- [ ] **Step 1: Create the struct** - -```csharp -namespace MergerLogic.ImageProcessing -{ - /// - /// Describes which inputs contributed to a merged tile, used to classify the - /// write as added / merged / replaced. TargetUsed is false in upload-only mode. - /// - public readonly struct MergeStats - { - public bool TargetUsed { get; } - public bool AnySourceUsed { get; } - - public MergeStats(bool targetUsed, bool anySourceUsed) - { - this.TargetUsed = targetUsed; - this.AnySourceUsed = anySourceUsed; - } - } -} -``` - -- [ ] **Step 2: Build** - -Run: `dotnet build GpkgMerger.sln` -Expected: succeeds. - -- [ ] **Step 3: Commit** - -```bash -git add MergerLogic/ImageProcessing/MergeStats.cs -git commit -m "feat: add MergeStats to describe merge tile provenance" -``` - ---- - -## Task 2: `MergeTiles` exposes `MergeStats` - -The target builder is index 0 of the `tiles` list. `GetImageList` iterates from the last source down to the target and short-circuits on the first fully-opaque tile. We must record whether the target image and any source image entered the stack. - -**Files:** -- Modify: `MergerLogic/ImageProcessing/ITileMerger.cs` -- Modify: `MergerLogic/ImageProcessing/TileMerger.cs` -- Test: `MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs` - -- [ ] **Step 1: Write failing tests for the stats out-param** - -Add to `TileMergerTest.cs` (inside the class): - -```csharp -[TestMethod] -[TestCategory("unit")] -[TestCategory("MergeTiles")] -public void MergeTilesStats_BlendedTargetAndSource_TargetAndSourceUsed() -{ - // transparent source over an existing target -> both contribute - var target = new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes()); - var source = new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes()); - var builders = new List { () => target, () => source }; - - this._testTileMerger.MergeTiles(builders, new Coord(0, 0, 0), - new TileFormatStrategy(TileFormat.Png), out MergeStats stats, uploadOnly: false); - - Assert.IsTrue(stats.TargetUsed); - Assert.IsTrue(stats.AnySourceUsed); -} - -[TestMethod] -[TestCategory("unit")] -[TestCategory("MergeTiles")] -public void MergeTilesStats_OpaqueSource_TargetNotUsed() -{ - var target = new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes()); - var opaqueSource = new Tile(new Coord(0, 0, 0), this.GetOpaquePngBytes()); - var builders = new List { () => target, () => opaqueSource }; - - this._testTileMerger.MergeTiles(builders, new Coord(0, 0, 0), - new TileFormatStrategy(TileFormat.Png), out MergeStats stats, uploadOnly: false); - - Assert.IsFalse(stats.TargetUsed); - Assert.IsTrue(stats.AnySourceUsed); -} - -[TestMethod] -[TestCategory("unit")] -[TestCategory("MergeTiles")] -public void MergeTilesStats_UploadOnly_TargetNotUsed() -{ - var target = new Tile(new Coord(0, 0, 0), this.GetOpaquePngBytes()); - var source = new Tile(new Coord(0, 0, 0), this.GetOpaquePngBytes()); - var builders = new List { () => target, () => source }; - - this._testTileMerger.MergeTiles(builders, new Coord(0, 0, 0), - new TileFormatStrategy(TileFormat.Png), out MergeStats stats, uploadOnly: true); - - Assert.IsFalse(stats.TargetUsed); - Assert.IsTrue(stats.AnySourceUsed); -} -``` - -Add these helpers to the test class if not already present (reuse existing test image bytes/fixtures in `Runners/TestData` or the existing `TileMergerTest` fixtures if they already expose transparent/opaque tiles — prefer the existing fixtures and delete these helpers if duplicative): - -```csharp -private byte[] GetTransparentPngBytes() => - File.ReadAllBytes(Path.Combine("TestData", "transparent.png")); -private byte[] GetOpaquePngBytes() => - File.ReadAllBytes(Path.Combine("TestData", "opaque.png")); -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `dotnet test --filter "ClassName~TileMergerTest&TestCategory=MergeTiles"` -Expected: FAIL to compile — no `MergeTiles` overload with `out MergeStats`. - -- [ ] **Step 3: Add the interface overload** - -Edit `ITileMerger.cs`: - -```csharp -using MergerLogic.Batching; -using MergerLogic.DataTypes; - -namespace MergerLogic.ImageProcessing -{ - public interface ITileMerger - { - Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, bool uploadOnly = false); - - Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, - out MergeStats stats, bool uploadOnly = false); - } -} -``` - -- [ ] **Step 4: Implement in `TileMerger.cs`** - -Replace the existing `MergeTiles` and `GetImageList` so provenance is tracked. Keep the old signature as a wrapper. - -```csharp -public Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, bool uploadOnly = false) -{ - return this.MergeTiles(tiles, targetCoords, strategy, out _, uploadOnly); -} - -public Tile? MergeTiles(List tiles, Coord targetCoords, TileFormatStrategy strategy, - out MergeStats stats, bool uploadOnly = false) -{ - bool targetUsed = false; - bool anySourceUsed = false; - - if (uploadOnly) - { - this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] Configured to upload only mode"); - // Ignore target in upload only mode - tiles = tiles.Skip(1).ToList(); - - if (tiles.Count == 1) - { - this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] Only one source was found, using raw image"); - Tile? rawTile = tiles[0](); - rawTile?.ConvertToFormat(strategy.ApplyStrategy(rawTile.Format)); - stats = new MergeStats(false, rawTile != null); - return rawTile; - } - } - - // hasTarget is true when the target builder (index 0) is still part of the list - bool hasTarget = !uploadOnly && tiles.Count > 0; - var images = this.GetImageList(tiles, targetCoords, uploadOnly, hasTarget, out targetUsed, out anySourceUsed); - IMagickImage image; - - switch (images.Count) - { - case 0: - this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] No images where found return null"); - stats = new MergeStats(targetUsed, anySourceUsed); - return null; - case 1: - ImageFormatter.RemoveImageDateAttributes(images[0]); - image = images[0]; - this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] 1 image found"); - break; - default: - using (var imageCollection = new MagickImageCollection()) - { - for (var i = images.Count - 1; i >= 0; i--) - { - imageCollection.Add(images[i]); - } - - this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] {imageCollection.Count} where found for merge, start 'imageMagic' merging"); - using (var mergedImage = imageCollection.Flatten(MagickColor.FromRgba(0, 0, 0, 0))) - { - ImageFormatter.RemoveImageDateAttributes(mergedImage); - mergedImage.ColorSpace = ColorSpace.sRGB; - mergedImage.ColorType = mergedImage.HasAlpha ? ColorType.TrueColorAlpha : ColorType.TrueColor; - image = new MagickImage(mergedImage); - this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] 'imageMagic' merging finished"); - } - } - break; - } - - Tile tile = new Tile(targetCoords, image); - image.Dispose(); - tile.ConvertToFormat(strategy.ApplyStrategy(tile.Format)); - stats = new MergeStats(targetUsed, anySourceUsed); - return tile; -} -``` - -Update `GetImageList` to report provenance. The loop index `i == 0` corresponds to the target when `hasTarget` is true. - -```csharp -private List GetImageList(List tiles, Coord targetCoords, bool uploadOnly, - bool hasTarget, out bool targetUsed, out bool anySourceUsed) -{ - var images = new List(); - int i = tiles.Count - 1; - Tile? tile = null; - targetUsed = false; - anySourceUsed = false; - - bool hasAlpha = false; - try - { - for (; i >= 0; i--) - { - // protect in case all "sources" tiles are null - if (images.Count == 0 && i == 0 && !uploadOnly) - { - this._logger.LogDebug($"[{MethodBase.GetCurrentMethod()?.Name}] All sources are empty - return"); - return images; - } - - tile = tiles[i](); - if (tile is null) - { - continue; - } - - int before = images.Count; - this.AddTileToImageList(targetCoords, tile, images, out hasAlpha); - bool added = images.Count > before; - if (added) - { - if (hasTarget && i == 0) - { - targetUsed = true; - } - else - { - anySourceUsed = true; - } - } - - if (!hasAlpha) - { - return images; - } - } - } - catch - { - images.ForEach(image => image.Dispose()); - throw; - } - - return images; -} -``` - -- [ ] **Step 5: Run tests to verify they pass** - -Run: `dotnet test --filter "ClassName~TileMergerTest&TestCategory=MergeTiles"` -Expected: PASS (all MergeTiles tests, including pre-existing ones). - -- [ ] **Step 6: Commit** - -```bash -git add MergerLogic/ImageProcessing/ITileMerger.cs MergerLogic/ImageProcessing/TileMerger.cs MergerLogicUnitTests/ImageProcessing/TileMergerTest.cs -git commit -m "feat: expose target/source provenance from MergeTiles via MergeStats" -``` - ---- - -## Task 3: `MergeReport` accumulator - -**Files:** -- Create: `MergerService/Models/Reports/MergeReport.cs` -- Test: `MergerServiceUnitTests/Models/MergeReportTest.cs` - -- [ ] **Step 1: Write failing tests** - -Create `MergerServiceUnitTests/Models/MergeReportTest.cs`: - -```csharp -using MergerLogic.DataTypes; -using MergerService.Models.Reports; -using Microsoft.VisualStudio.TestTools.UnitTesting; -using Newtonsoft.Json.Linq; -using System; - -namespace MergerServiceUnitTests.Models -{ - [TestClass] - [TestCategory("unit")] - public class MergeReportTest - { - [TestMethod] - public void Counts_And_Percentages_Are_Computed() - { - var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); - report.RecordAdded(new Coord(10, 1, 2)); - report.RecordAdded(new Coord(10, 1, 3)); - report.RecordMerged(); - report.RecordReplaced(); - report.RecordSkipped(); - - report.Finalize(new DateTime(2026, 9, 15, 0, 0, 0, DateTimeKind.Utc), - new DateTime(2026, 9, 15, 0, 1, 0, DateTimeKind.Utc)); - - Assert.AreEqual(2, report.Added); - Assert.AreEqual(1, report.Merged); - Assert.AreEqual(1, report.Replaced); - Assert.AreEqual(1, report.Skipped); - Assert.AreEqual(5, report.Total); - Assert.AreEqual(60, report.DurationSeconds); - Assert.AreEqual(40.0, report.AddedPercentage, 0.01); - } - - [TestMethod] - public void Json_Includes_AddedTiles_LogString_Excludes_Them() - { - var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); - report.RecordAdded(new Coord(10, 1, 2)); - report.Finalize(DateTime.UnixEpoch, DateTime.UnixEpoch); - - JObject json = JObject.Parse(report.ToJson()); - Assert.AreEqual(1, ((JArray)json["addedTiles"]).Count); - - Assert.IsFalse(report.ToLogString().Contains("addedTiles")); - Assert.IsTrue(report.ToLogString().Contains("\"added\"")); - } - - [TestMethod] - public void Percentages_Are_Zero_When_No_Tiles() - { - var report = new MergeReport("job1", "task1", "MERGE", "PNG", true); - report.Finalize(DateTime.UnixEpoch, DateTime.UnixEpoch); - Assert.AreEqual(0, report.Total); - Assert.AreEqual(0.0, report.AddedPercentage, 0.01); - } - } -} -``` - -- [ ] **Step 2: Run to verify failure** - -Run: `dotnet test --filter "ClassName~MergeReportTest"` -Expected: FAIL to compile — `MergeReport` does not exist. - -- [ ] **Step 3: Implement `MergeReport`** - -Create `MergerService/Models/Reports/MergeReport.cs`: - -```csharp -using MergerLogic.DataTypes; -using Newtonsoft.Json; -using Newtonsoft.Json.Serialization; - -namespace MergerService.Models.Reports -{ - public class MergeReport - { - public int Version => 1; - public string JobId { get; } - public string TaskId { get; } - public string TaskType { get; } - public string TargetFormat { get; } - public bool IsNewTarget { get; } - - public DateTime StartTime { get; private set; } - public DateTime EndTime { get; private set; } - public double DurationSeconds { get; private set; } - - public int Added { get; private set; } - public int Merged { get; private set; } - public int Replaced { get; private set; } - public int Skipped { get; private set; } - public int Total => this.Added + this.Merged + this.Replaced + this.Skipped; - - public double AddedPercentage { get; private set; } - public double MergedPercentage { get; private set; } - public double ReplacedPercentage { get; private set; } - public double SkippedPercentage { get; private set; } - - public List AddedTiles { get; } = new List(); - - public MergeReport(string jobId, string taskId, string taskType, string targetFormat, bool isNewTarget) - { - this.JobId = jobId; - this.TaskId = taskId; - this.TaskType = taskType; - this.TargetFormat = targetFormat; - this.IsNewTarget = isNewTarget; - } - - public void RecordAdded(Coord coord) - { - this.Added++; - this.AddedTiles.Add(coord); - } - - public void RecordMerged() => this.Merged++; - public void RecordReplaced() => this.Replaced++; - public void RecordSkipped() => this.Skipped++; - - public void Finalize(DateTime startTime, DateTime endTime) - { - this.StartTime = startTime; - this.EndTime = endTime; - this.DurationSeconds = (endTime - startTime).TotalSeconds; - - int total = this.Total; - if (total > 0) - { - this.AddedPercentage = 100.0 * this.Added / total; - this.MergedPercentage = 100.0 * this.Merged / total; - this.ReplacedPercentage = 100.0 * this.Replaced / total; - this.SkippedPercentage = 100.0 * this.Skipped / total; - } - } - - // camelCase so the artifact matches the spec JSON shape (jobId, counts, z/x/y) - public string ToJson() => JsonConvert.SerializeObject(this, Formatting.None, - new JsonSerializerSettings { ContractResolver = new CamelCasePropertyNamesContractResolver() }); - - // Summary for the structured log line: counts + percentages, WITHOUT the added-tile list. - public string ToLogString() - { - var summary = new - { - version = this.Version, - jobId = this.JobId, - taskId = this.TaskId, - taskType = this.TaskType, - targetFormat = this.TargetFormat, - isNewTarget = this.IsNewTarget, - durationSeconds = this.DurationSeconds, - counts = new { added = this.Added, merged = this.Merged, replaced = this.Replaced, skipped = this.Skipped, total = this.Total }, - percentages = new { added = this.AddedPercentage, merged = this.MergedPercentage, replaced = this.ReplacedPercentage, skipped = this.SkippedPercentage } - }; - return JsonConvert.SerializeObject(summary, Formatting.None); - } - } -} -``` - -Note: `ToJson()` uses `CamelCasePropertyNamesContractResolver`, so all property/field names serialize camelCase (`jobId`, `addedTiles`, and `Coord` → `z/x/y`), matching the spec's artifact shape. The explicit `[JsonProperty("addedTiles")]` on `AddedTiles` is kept but redundant under the resolver. - -- [ ] **Step 4: Run to verify pass** - -Run: `dotnet test --filter "ClassName~MergeReportTest"` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add MergerService/Models/Reports/MergeReport.cs MergerServiceUnitTests/Models/MergeReportTest.cs -git commit -m "feat: add MergeReport accumulator with counts, percentages and added-tile list" -``` - ---- - -## Task 4: `IReportWriter` + `ReportWriter` (FS/S3 sink) - -Writer serializes a `MergeReport` and writes it to a file named `merge-report-{jobId}-{taskId}.json` under `outputPath`. Sink chosen by config `REPORT:sink` (`"FS"` or `"S3"`). For S3 the bucket comes from `S3:bucket` and `outputPath` is the key prefix. Absent/empty `outputPath` → no-op. Write failures throw (caller decides how to handle — see the open question in the spec). - -**Files:** -- Create: `MergerService/Utils/IReportWriter.cs` -- Create: `MergerService/Utils/ReportWriter.cs` -- Test: `MergerServiceUnitTests/Utils/ReportWriterTest.cs` - -- [ ] **Step 1: Write failing tests** - -Create `MergerServiceUnitTests/Utils/ReportWriterTest.cs`: - -```csharp -using Amazon.S3; -using Amazon.S3.Model; -using MergerLogic.Utils; -using MergerService.Models.Reports; -using MergerService.Utils; -using Microsoft.Extensions.Logging; -using Microsoft.VisualStudio.TestTools.UnitTesting; -using Moq; -using System.IO.Abstractions; -using System.IO.Abstractions.TestingHelpers; -using System.Threading; -using System.Threading.Tasks; - -namespace MergerServiceUnitTests.Utils -{ - [TestClass] - [TestCategory("unit")] - public class ReportWriterTest - { - private Mock _config; - private Mock> _logger; - private Mock _s3; - - [TestInitialize] - public void BeforeEach() - { - this._config = new Mock(MockBehavior.Loose); - this._logger = new Mock>(MockBehavior.Loose); - this._s3 = new Mock(MockBehavior.Loose); - } - - private MergeReport BuildReport() - { - var r = new MergeReport("job1", "task1", "MERGE", "PNG", false); - r.Finalize(System.DateTime.UnixEpoch, System.DateTime.UnixEpoch); - return r; - } - - [TestMethod] - public void FsSink_WritesJsonFile() - { - this._config.Setup(c => c.GetConfiguration("REPORT", "sink")).Returns("FS"); - var fs = new MockFileSystem(); - var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); - - writer.WriteReport(this.BuildReport(), "/reports"); - - string expected = fs.Path.Combine("/reports", "merge-report-job1-task1.json"); - Assert.IsTrue(fs.FileExists(expected)); - StringAssert.Contains(fs.File.ReadAllText(expected), "\"jobId\":\"job1\""); - } - - [TestMethod] - public void S3Sink_PutsObject() - { - this._config.Setup(c => c.GetConfiguration("REPORT", "sink")).Returns("S3"); - this._config.Setup(c => c.GetConfiguration("S3", "bucket")).Returns("tiles"); - this._s3.Setup(s => s.PutObjectAsync(It.IsAny(), It.IsAny())) - .ReturnsAsync(new PutObjectResponse()); - var fs = new MockFileSystem(); - var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); - - writer.WriteReport(this.BuildReport(), "reports"); - - this._s3.Verify(s => s.PutObjectAsync( - It.Is(r => r.BucketName == "tiles" && r.Key == "reports/merge-report-job1-task1.json"), - It.IsAny()), Times.Once); - } - - [TestMethod] - public void EmptyPath_IsNoOp() - { - var fs = new MockFileSystem(); - var writer = new ReportWriter(this._config.Object, fs, this._s3.Object, this._logger.Object); - - writer.WriteReport(this.BuildReport(), null); - writer.WriteReport(this.BuildReport(), ""); - - Assert.AreEqual(0, fs.AllFiles.Count()); - this._s3.Verify(s => s.PutObjectAsync(It.IsAny(), It.IsAny()), Times.Never); - } - } -} -``` - -Ensure the test project references `System.IO.Abstractions.TestingHelpers` (already used elsewhere in the suite; if the package is missing, add `` to `MergerServiceUnitTests.csproj` matching the version of `System.IO.Abstractions` already referenced). - -- [ ] **Step 2: Run to verify failure** - -Run: `dotnet test --filter "ClassName~ReportWriterTest"` -Expected: FAIL to compile — `IReportWriter`/`ReportWriter` do not exist. - -- [ ] **Step 3: Implement interface** - -Create `MergerService/Utils/IReportWriter.cs`: - -```csharp -using MergerService.Models.Reports; - -namespace MergerService.Utils -{ - public interface IReportWriter - { - // Writes the report JSON artifact to the configured sink under outputPath. - // No-op when outputPath is null/empty. Throws on write failure. - void WriteReport(MergeReport report, string? outputPath); - } -} -``` - -- [ ] **Step 4: Implement writer** - -Create `MergerService/Utils/ReportWriter.cs`: - -```csharp -using Amazon.S3; -using Amazon.S3.Model; -using MergerLogic.Utils; -using MergerService.Models.Reports; -using Microsoft.Extensions.Logging; -using System.IO.Abstractions; -using System.Reflection; -using System.Text; - -namespace MergerService.Utils -{ - public class ReportWriter : IReportWriter - { - private readonly IConfigurationManager _configuration; - private readonly IFileSystem _fileSystem; - private readonly IAmazonS3 _s3; - private readonly ILogger _logger; - - public ReportWriter(IConfigurationManager configuration, IFileSystem fileSystem, IAmazonS3 s3, - ILogger logger) - { - this._configuration = configuration; - this._fileSystem = fileSystem; - this._s3 = s3; - this._logger = logger; - } - - public void WriteReport(MergeReport report, string? outputPath) - { - string methodName = MethodBase.GetCurrentMethod().Name; - if (string.IsNullOrEmpty(outputPath)) - { - this._logger.LogDebug($"[{methodName}] No ReportOutputPath configured, skipping report artifact"); - return; - } - - string fileName = $"merge-report-{report.JobId}-{report.TaskId}.json"; - string json = report.ToJson(); - string sink = this._configuration.GetConfiguration("REPORT", "sink"); - - if (string.Equals(sink, "S3", System.StringComparison.OrdinalIgnoreCase)) - { - this.WriteToS3(outputPath, fileName, json); - } - else - { - this.WriteToFs(outputPath, fileName, json); - } - - this._logger.LogInformation($"[{methodName}] Wrote merge report to {sink}:{outputPath}/{fileName}"); - } - - private void WriteToFs(string outputPath, string fileName, string json) - { - this._fileSystem.Directory.CreateDirectory(outputPath); - string fullPath = this._fileSystem.Path.Combine(outputPath, fileName); - this._fileSystem.File.WriteAllText(fullPath, json); - } - - private void WriteToS3(string outputPath, string fileName, string json) - { - string bucket = this._configuration.GetConfiguration("S3", "bucket"); - string key = $"{outputPath.TrimEnd('/')}/{fileName}"; - var request = new PutObjectRequest - { - BucketName = bucket, - Key = key, - ContentBody = json, - ContentType = "application/json" - }; - var res = this._s3.PutObjectAsync(request).Result; - } - } -} -``` - -- [ ] **Step 5: Run to verify pass** - -Run: `dotnet test --filter "ClassName~ReportWriterTest"` -Expected: PASS. - -- [ ] **Step 6: Commit** - -```bash -git add MergerService/Utils/IReportWriter.cs MergerService/Utils/ReportWriter.cs MergerServiceUnitTests/Utils/ReportWriterTest.cs -git commit -m "feat: add ReportWriter with FS and S3 sinks for merge report artifact" -``` - ---- - -## Task 5: `ReportOutputPath` on `AdditionalParams` - -**Files:** -- Modify: `MergerService/Models/Jobs/JobParamersAdditiomalParams.cs` - -- [ ] **Step 1: Add the nullable field + ctor param** - -Edit `AdditionalParams` to add the property and an optional constructor parameter (optional so existing construction sites and deserialization keep working): - -```csharp -public class AdditionalParams -{ - [JsonInclude] public string? JobTrackerServiceURL { get; } - [JsonInclude] public string? ReportOutputPath { get; } - - [System.Text.Json.Serialization.JsonIgnore] - private JsonSerializerSettings _jsonSerializerSettings; - - public AdditionalParams(string jobTrackerServiceURL, string? reportOutputPath = null) - { - this.JobTrackerServiceURL = jobTrackerServiceURL; - this.ReportOutputPath = reportOutputPath; - - this._jsonSerializerSettings = new JsonSerializerSettings(); - this._jsonSerializerSettings.Converters.Add(new StringEnumConverter()); - } -} -``` - -- [ ] **Step 2: Build** - -Run: `dotnet build GpkgMerger.sln` -Expected: succeeds. - -- [ ] **Step 3: Commit** - -```bash -git add MergerService/Models/Jobs/JobParamersAdditiomalParams.cs -git commit -m "feat: add ReportOutputPath to job AdditionalParams" -``` - ---- - -## Task 6: Classify, accumulate, and emit in `TaskExecutor` - -Add `IReportWriter` dependency, extend `ExecuteTask` with `reportOutputPath`, build a `MergeReport`, classify each tile, then finalize + log + write. - -**Files:** -- Modify: `MergerService/Runners/ITaskExecutor.cs` -- Modify: `MergerService/Runners/TaskExecutor.cs` -- Test: `MergerServiceUnitTests/Runners/TaskExecutorTest.cs` - -- [ ] **Step 1: Write a failing classification test** - -Add to `TaskExecutorTest.cs`. This drives a 1-tile batch where the target already exists and the merged tile uses the target → expect one `RecordMerged` and a report artifact write. Use the existing test's mock wiring for `IDataFactory`/`IData`; add an `IReportWriter` mock and assert it is called once with a report whose `Merged == 1`. - -```csharp -[TestMethod] -[TestCategory("unit")] -[TestCategory("runners")] -public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() -{ - // Arrange: target.TileExists(coord) == true, MergeTiles returns a tile with TargetUsed=true. - var reportWriterMock = this._mockRepository.Create(); - MergeReport captured = null; - reportWriterMock - .Setup(w => w.WriteReport(It.IsAny(), "reports")) - .Callback((r, p) => captured = r); - - var target = new Mock(MockBehavior.Loose); - target.SetupGet(t => t.Type).Returns(DataType.GPKG); - target.Setup(t => t.TileExists(It.IsAny())).Returns(true); - target.Setup(t => t.GetCorrespondingTile(It.IsAny(), It.IsAny())) - .Returns(new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes())); - // ... wire _dataFactoryMock.CreateDataSource(...) to return target for a single-source task - // and metadata with IsNewTarget=false and a single 1x1 batch at z0. - - var executor = this.BuildExecutor(reportWriterMock.Object); // helper that constructs TaskExecutor with all mocks - - // Act - executor.ExecuteTask(BuildSingleTileTask(isNewTarget: false), this._taskUtilsMock.Object, null, "reports"); - - // Assert - reportWriterMock.Verify(w => w.WriteReport(It.IsAny(), "reports"), Times.Once); - Assert.IsNotNull(captured); - Assert.AreEqual(1, captured.Merged); - Assert.AreEqual(0, captured.Added); -} -``` - -Add two sibling tests mirroring this exactly but for the other branches (repeat the arrange block; do not cross-reference): - -- `ExecuteTask_NewTargetTile_CountsAsAdded`: `target.TileExists` returns `false` (or `IsNewTarget=true`), `MergeTiles` yields `AnySourceUsed=true`, `TargetUsed=false`; assert `captured.Added == 1`, `captured.AddedTiles.Count == 1`. -- `ExecuteTask_OpaqueSourceOverExisting_CountsAsReplaced`: `target.TileExists` returns `true`, `MergeTiles` yields `TargetUsed=false`, `AnySourceUsed=true`; assert `captured.Replaced == 1`. - -To make the merge outcome deterministic in these tests, mock `ITileMerger` instead of using the real one, so the test controls the returned `MergeStats`: - -```csharp -this._tileMergerMock = this._mockRepository.Create(); -MergeStats outStats = new MergeStats(targetUsed: false, anySourceUsed: true); -this._tileMergerMock - .Setup(m => m.MergeTiles(It.IsAny>(), It.IsAny(), - It.IsAny(), out outStats, It.IsAny())) - .Returns(new Tile(new Coord(0, 0, 0), this.GetTransparentPngBytes())); -``` - -(Replace the currently-used real `_testTileMerger` in the executor construction with `_tileMergerMock.Object` for these tests.) - -- [ ] **Step 2: Run to verify failure** - -Run: `dotnet test --filter "ClassName~TaskExecutorTest&TestCategory=runners"` -Expected: FAIL to compile — `ExecuteTask` has no `reportOutputPath` param and `TaskExecutor` ctor has no `IReportWriter`. - -- [ ] **Step 3: Update the interface** - -Edit `ITaskExecutor.cs`: - -```csharp -using MergerService.Models.Tasks; -using MergerService.Utils; - -namespace MergerService.Runners -{ - public interface ITaskExecutor - { - void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl, string? reportOutputPath); - } -} -``` - -**IMPORTANT — existing call sites to update (compile breakers):** -- `MergerServiceUnitTests/Runners/TaskExecutorTest.cs` constructs `TaskExecutor` in **two** places (in `WhenGivenSourcesWithOneTile...` and `WhenConfiguringBatchLimits...`). Add an `IReportWriter` mock in `BeforeEach` and pass `reportWriterMock.Object` as the new final ctor arg at both sites. -- The same two tests call `ExecuteTask(testTask, _taskUtilsMock.Object, null)` — add the 4th arg → `ExecuteTask(testTask, _taskUtilsMock.Object, null, null)`. -- `MockRepository` is `Loose` and there is no `VerifyAll()`, so the new `target.TileExists(coord)` call in `ExecuteTask` (unmocked → returns `false`) does not break these existing tests. - -- [ ] **Step 4: Wire the writer + report into `TaskExecutor`** - -In `TaskExecutor.cs`: - -1. Add field + ctor param: - -```csharp -private readonly IReportWriter _reportWriter; -``` - -Add `IReportWriter reportWriter` to the constructor signature and assign `this._reportWriter = reportWriter;`. - -2. Add `using MergerService.Models.Reports;` at the top. - -3. Change the method signature: - -```csharp -public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCallbackUrl, string? reportOutputPath) -``` - -4. After `MergeMetadata metadata = task.Parameters;` create the report and capture start time: - -```csharp -DateTime reportStart = DateTime.UtcNow; -MergeReport report = new MergeReport(task.JobId, task.Id, task.Type, - metadata.TargetFormat.ToString(), metadata.IsNewTarget); -``` - -5. Replace the per-coord merge block. Currently: - -```csharp -var tileMergeStopwatch = Stopwatch.StartNew(); -Tile? tile = this._tileMerger.MergeTiles(correspondingTileBuilders, coord, strategy, metadata.IsNewTarget); -tileMergeStopwatch.Stop(); -this._metricsProvider.MergeTimePerTileHistogram(tileMergeStopwatch.Elapsed.TotalSeconds, metadata.TargetFormat); - -if (tile != null) -{ - tiles.Add(tile); - currentBatchBytes += tile.Size(); - ... -} -``` - -Change to classify before/after merge: - -```csharp -bool existedBefore = !metadata.IsNewTarget && target.TileExists(coord); - -var tileMergeStopwatch = Stopwatch.StartNew(); -Tile? tile = this._tileMerger.MergeTiles(correspondingTileBuilders, coord, strategy, out MergeStats stats, metadata.IsNewTarget); -tileMergeStopwatch.Stop(); -this._metricsProvider.MergeTimePerTileHistogram(tileMergeStopwatch.Elapsed.TotalSeconds, metadata.TargetFormat); - -if (tile != null) -{ - if (!stats.AnySourceUsed) - { - // target re-encode with no source data — not a real change - report.RecordSkipped(); - } - else if (!existedBefore) - { - report.RecordAdded(coord); - } - else if (stats.TargetUsed) - { - report.RecordMerged(); - } - else - { - report.RecordReplaced(); - } - - tiles.Add(tile); - currentBatchBytes += tile.Size(); - - if (currentBatchBytes >= this._batchMaxBytes || (this._limitBatchSize && tiles.Count >= this._batchMaxSize)) - { - this.UpdateTargetTiles(target, tiles, task, overallTileProgressCount, totalTileCount, taskUtils); - tiles.Clear(); - currentBatchBytes = 0; - } -} -else -{ - report.RecordSkipped(); -} -``` - -6. After `target.Wrapup();` (still inside `ExecuteTask`, before the final debug log), finalize + emit: - -```csharp -report.Finalize(reportStart, DateTime.UtcNow); -this._logger.LogInformation($"[{methodName}] Merge report: {report.ToLogString()}"); -try -{ - this._reportWriter.WriteReport(report, reportOutputPath); -} -catch (Exception e) -{ - // Best-effort (proposed default). Whether this should fail the task is an open - // question raised on the implementation PR. - this._logger.LogError(e, $"[{methodName}] Failed to write merge report artifact: {e.Message}"); -} -``` - -- [ ] **Step 5: Run to verify pass** - -Run: `dotnet test --filter "ClassName~TaskExecutorTest&TestCategory=runners"` -Expected: PASS. - -- [ ] **Step 6: Commit** - -```bash -git add MergerService/Runners/ITaskExecutor.cs MergerService/Runners/TaskExecutor.cs MergerServiceUnitTests/Runners/TaskExecutorTest.cs -git commit -m "feat: classify added/merged/replaced tiles and emit merge report in TaskExecutor" -``` - ---- - -## Task 7: Pass `ReportOutputPath` through `TaskRunner` - -**Files:** -- Modify: `MergerService/Runners/TaskRunner.cs` -- Test: `MergerServiceUnitTests/Runners/TaskRunnerTest.cs` - -- [ ] **Step 1: Write/adjust failing test** - -`RunTask` currently calls `this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl)`. The `ITaskExecutor` change (Task 6) breaks compilation of `TaskRunner` and its tests. Update the `TaskRunnerTest` verification of `ExecuteTask` to include the new argument, and add an assertion that the `ReportOutputPath` from the job flows through: - -```csharp -// In the existing "happy path" RunTask test where a job is returned by _jobUtils: -this._taskExecutorMock.Verify(e => e.ExecuteTask(task, this._taskUtils, It.IsAny(), "reports"), Times.Once); -``` - -Set the mocked job's `AdditionalParams.ReportOutputPath` to `"reports"` in that test's job fixture. - -- [ ] **Step 2: Run to verify failure** - -Run: `dotnet test --filter "ClassName~TaskRunnerTest"` -Expected: FAIL (compile or verification mismatch). - -- [ ] **Step 3: Implement pass-through** - -In `TaskRunner.RunTask`, where `managerCallbackUrl` is derived, also derive the report path from the same job, then pass it: - -```csharp -MergeJob? job = this._jobUtils.GetJob(task.JobId); -string? managerCallbackUrl = job?.Parameters.AdditionalParams?.JobTrackerServiceURL; -string? reportOutputPath = job?.Parameters.AdditionalParams?.ReportOutputPath; -``` - -(There is currently a single inline `this._jobUtils.GetJob(task.JobId)?...` call for `managerCallbackUrl`; replace it with the `job` local above so the job is fetched once.) Then: - -```csharp -this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl, reportOutputPath); -``` - -Add `using MergerService.Models.Jobs;` if not already present. - -- [ ] **Step 4: Run to verify pass** - -Run: `dotnet test --filter "ClassName~TaskRunnerTest"` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add MergerService/Runners/TaskRunner.cs MergerServiceUnitTests/Runners/TaskRunnerTest.cs -git commit -m "feat: pass job ReportOutputPath from TaskRunner to TaskExecutor" -``` - ---- - -## Task 8: Register `IReportWriter` + add `REPORT` config - -**Files:** -- Modify: `MergerService/Program.cs` -- Modify: `MergerService/appsettings.json` - -- [ ] **Step 1: Register the writer** - -In `Program.cs`, after `builder.Services.AddSingleton();` add: - -```csharp -builder.Services.AddSingleton(); -``` - -Add `using MergerService.Utils;` if not already imported. `IFileSystem`, `IAmazonS3`, and `IConfigurationManager` are already registered via `RegisterMergerLogicType()`. - -- [ ] **Step 2: Add config section** - -In `appsettings.json`, add a top-level `REPORT` section: - -```json - "REPORT": { - "sink": "FS" - }, -``` - -- [ ] **Step 3: Build** - -Run: `dotnet build GpkgMerger.sln` -Expected: succeeds. - -- [ ] **Step 4: Commit** - -```bash -git add MergerService/Program.cs MergerService/appsettings.json -git commit -m "build: register ReportWriter and add REPORT sink config" -``` - ---- - -## Task 9: Full build + test sweep - -**Files:** none. - -- [ ] **Step 1: Build the solution** - -Run: `dotnet build GpkgMerger.sln` -Expected: succeeds with no errors. - -- [ ] **Step 2: Run the full unit-test suite** - -Run: `dotnet test GpkgMerger.sln --filter "TestCategory=unit"` -Expected: all tests pass, including the pre-existing `TileMergerTest`, `TaskExecutorTest`, and `TaskRunnerTest`. - -- [ ] **Step 3: Verify the CLI still compiles against `ITileMerger`** - -`MergerCli/Process.cs` uses the original `MergeTiles(...)` overload, which is preserved. Confirm it builds (covered by Step 1). No change required. - -- [ ] **Step 4: Post the open question on the PR** - -After opening the implementation PR, post the error-handling open question from the spec (`## Error handling — OPEN QUESTION`) as a PR comment so reviewers decide whether report-write failure should stay best-effort or fail the task. - ---- - -## Post-implementation - -- Follow-up ticket **MAPCO-11688** covers exposing these counts as Prometheus metrics + dashboard and auditing/reorganizing existing metrics/dashboards. Not part of this plan. diff --git a/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md b/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md deleted file mode 100644 index c1ee535..0000000 --- a/docs/superpowers/specs/2026-09-15-tile-merge-report-design.md +++ /dev/null @@ -1,157 +0,0 @@ -# Tile Merge Report — Design - -Date: 2026-09-15 -Status: Approved for planning - -## Goal - -Produce a per-task report of what a merge did to the target: - -- **Counts**: added, merged (changed), replaced tiles, plus total. -- **Added-tiles list**: exact `z/x/y` of every tile that did not exist in the target - before the merge. -- Emitted as a **structured log line** (counts + percentages, no list) and a **JSON - artifact file** (full detail incl. the added list). - -Definitions (authoritative): - -- **Added** — tile did not exist in the target before the merge. -- **Merged (changed)** — tile existed and the target pixels were blended with source - pixels (alpha composite). -- **Replaced (full replace)** — tile existed but a fully-opaque source covered it; the - target tile was not blended in. - -## Scope - -- **Per task** (the current execution unit in `TaskExecutor.ExecuteTask`). Job-level - aggregation is a follow-up handled by the job tracker / dashboards. -- No change to merge output tiles themselves — this is observability only. - -## Classification - -Done inside the coord loop of `TaskExecutor.ExecuteTask`, per coord: - -``` -existedBefore = !metadata.IsNewTarget && target.TileExists(coord) // exact coord, no upscale -tile = MergeTiles(builders, coord, strategy, out MergeStats stats) -``` - -`MergeStats` (new, returned by `MergeTiles`): - -- `TargetUsed` — the target tile was included in the flattened image stack. -- `AnySourceUsed` — at least one source tile contributed data. - -Classification: - -| existedBefore | tile | TargetUsed | Result | -|---------------|-------------|------------|----------| -| — | `null` | — | skipped (no write, counted as `skipped`) | -| false | non-null | — | **Added** (record `z/x/y`) | -| true | non-null | true | **Merged** | -| true | non-null | false | **Replaced** | - -Notes / edge cases: - -- `TileExists` is skipped when `IsNewTarget` (target created empty → everything is - Added). This avoids one existence lookup per tile on new targets. -- `TileExists` checks the **exact** coord (no upscale), matching the "exact tile did - not exist" definition. The target builder in the merge uses upscaling, so its - non-null result is not a reliable existence signal — hence the separate check. -- A tile where the target exists but no source contributed data - (`TargetUsed && !AnySourceUsed`) is a target-only re-encode with no real change. - It is counted as `skipped`, not `merged`. (Whether such tiles should be written at - all is pre-existing behavior and out of scope.) - -## Components - -### 1. `MergeStats` + `MergeTiles` signal (MergerLogic/ImageProcessing) - -- Add `MergeStats` struct (`TargetUsed`, `AnySourceUsed`). -- Add `ITileMerger.MergeTiles(..., out MergeStats stats)`. -- Keep the existing `Tile? MergeTiles(...)` as a thin wrapper that discards the stats, - so `MergerCli/Process.cs` and existing `TileMergerTest` are untouched. -- Populate the stats in `GetImageList`: `TargetUsed` = the target builder (index 0) - produced an image that entered the stack; `AnySourceUsed` = any non-target image - entered the stack. Respect the opaque short-circuit and `uploadOnly` (target dropped - → `TargetUsed` always false). - -### 2. `MergeReport` accumulator (MergerService) - -- Plain object created per task. Fields: `jobId`, `taskId`, `taskType`, - `targetFormat`, `isNewTarget`, `startTime`, `endTime`, counts - (`added`, `merged`, `replaced`, `skipped`, `total`), and `List addedTiles`. -- Incremented in the sequential coord loop (no locking needed). -- Computes percentages of `total` per category at finalize. -- Includes a schema `version` field for forward compatibility. - -### 3. `IReportWriter` (MergerService) - -- Serializes the finalized `MergeReport` to JSON and writes it to the configured sink. -- Destination path = `AdditionalParams.ReportOutputPath` on the job object. -- Sink type (S3 vs FS) chosen by service configuration; reuses existing S3/File client - patterns in `MergerLogic/Clients`. -- If `ReportOutputPath` is absent/empty → skip the artifact; still emit the log line. -- Failure to write the artifact must not fail the task — log an error and continue. - -### 4. Wiring - -- Add `ReportOutputPath` (nullable string) to - `MergerService/Models/Jobs/JobParamersAdditiomalParams.cs` (`AdditionalParams`). -- `TaskRunner.RunTask` already reads `job.Parameters.AdditionalParams`; extract - `ReportOutputPath` there and pass it into `ExecuteTask` alongside `managerCallbackUrl`. -- `TaskExecutor.ExecuteTask` builds and finalizes the `MergeReport`, then calls - `IReportWriter` and emits the structured log line. - -## Report JSON shape (artifact) - -```json -{ - "version": 1, - "jobId": "...", - "taskId": "...", - "taskType": "...", - "targetFormat": "PNG", - "isNewTarget": false, - "startTime": "2026-09-15T00:00:00Z", - "endTime": "2026-09-15T00:01:00Z", - "durationSeconds": 60, - "counts": { "added": 10, "merged": 5, "replaced": 2, "skipped": 1, "total": 18 }, - "percentages": { "added": 55.6, "merged": 27.8, "replaced": 11.1, "skipped": 5.6 }, - "addedTiles": [ { "z": 10, "x": 1, "y": 2 } ] -} -``` - -Log line = same object **without** `addedTiles`. - -## Error handling — OPEN QUESTION (raise on the implementation PR) - -Do **not** bake this in as decided. Post it as an open-question comment on the -implementation PR for reviewer input: - -> Should a failure to write the report artifact (serialization / sink error) be -> best-effort (log + swallow, task still succeeds), or should it fail/reject the task? -> Best-effort is the proposed default, but the report may be a downstream dependency — -> confirm before finalizing. - -- Absent `ReportOutputPath` → log line only (not in question; this is settled). - -## Testing - -- Classification branches (added / merged / replaced / skipped) driven by `MergeStats` - and `existedBefore`. -- `MergeStats` population in `TileMerger` (target-only, opaque source, blended, uploadOnly). -- `MergeReport` percentage math and totals. -- `IReportWriter`: FS write, S3 write, absent-path skip, write-failure is non-fatal. - -## Out of scope - -- Job-level aggregation across tasks (tracker/dashboard concern). -- Emitting counts as Prometheus metrics and dashboard work — tracked in a separate - Jira ticket (see below), which also covers auditing/cleaning existing metrics and - organizing the dashboards. - -## Follow-up ticket - -**MAPCO-11688** — add added/merged/replaced counts (and percentage-of-total per -category) as Prometheus metrics and to the dashboard; audit, clean up, and reorganize -existing metrics + dashboards. From 07a5eb066a491dced613bc3dae6a057f3a59a56e Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Tue, 15 Sep 2026 18:00:27 +0300 Subject: [PATCH 17/27] refactor: encapsulate tile classification in MergeReport.RecordOutcome Move the added/merged/replaced/skipped decision out of TaskExecutor into MergeReport.RecordOutcome; make the Record* counters private. Classification permutations now unit-tested directly on MergeReport; TaskExecutorTest keeps one end-to-end wiring test. Co-Authored-By: Claude Opus 4.8 (1M context) --- MergerService/Models/Reports/MergeReport.cs | 39 ++- MergerService/Runners/TaskExecutor.cs | 24 +- .../Models/MergeReportTest.cs | 73 +++++- .../Runners/TaskExecutorTest.cs | 226 +----------------- 4 files changed, 110 insertions(+), 252 deletions(-) diff --git a/MergerService/Models/Reports/MergeReport.cs b/MergerService/Models/Reports/MergeReport.cs index 4129573..ad892f6 100644 --- a/MergerService/Models/Reports/MergeReport.cs +++ b/MergerService/Models/Reports/MergeReport.cs @@ -1,4 +1,5 @@ using MergerLogic.DataTypes; +using MergerLogic.ImageProcessing; using Newtonsoft.Json; using Newtonsoft.Json.Serialization; @@ -40,15 +41,43 @@ public MergeReport(string jobId, string taskId, string taskType, string targetFo this.IsNewTarget = isNewTarget; } - public void RecordAdded(Coord coord) + // Classifies a single merged tile into added / merged / replaced / skipped. + // added = tile didn't exist in target before the merge; merged = existed and the + // target was blended with source data; replaced = existed and an opaque source + // covered it; skipped = no tile produced or no source data contributed. + public void RecordOutcome(Coord coord, bool existedBefore, bool tileProduced, MergeStats stats) + { + if (!tileProduced || !stats.AnySourceUsed) + { + this.RecordSkipped(); + return; + } + + if (!existedBefore) + { + this.RecordAdded(coord); + return; + } + + if (stats.TargetUsed) + { + this.RecordMerged(); + return; + } + + this.RecordReplaced(); + } + + private void RecordAdded(Coord coord) { this.Added++; - this.AddedTiles.Add(coord); + // store a copy so a later in-place coord mutation can't corrupt the list + this.AddedTiles.Add(new Coord(coord.Z, coord.X, coord.Y)); } - public void RecordMerged() => this.Merged++; - public void RecordReplaced() => this.Replaced++; - public void RecordSkipped() => this.Skipped++; + private void RecordMerged() => this.Merged++; + private void RecordReplaced() => this.Replaced++; + private void RecordSkipped() => this.Skipped++; public void Finalize(DateTime startTime, DateTime endTime) { diff --git a/MergerService/Runners/TaskExecutor.cs b/MergerService/Runners/TaskExecutor.cs index 8b73692..85a6240 100644 --- a/MergerService/Runners/TaskExecutor.cs +++ b/MergerService/Runners/TaskExecutor.cs @@ -177,26 +177,10 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal tileMergeStopwatch.Stop(); this._metricsProvider.MergeTimePerTileHistogram(tileMergeStopwatch.Elapsed.TotalSeconds, metadata.TargetFormat); + report.RecordOutcome(coord, existedBefore, tile != null, stats); + if (tile != null) { - if (!stats.AnySourceUsed) - { - // No source contributed data → not counted as a real change (skipped), even if the target coord was empty before. - report.RecordSkipped(); - } - else if (!existedBefore) - { - report.RecordAdded(new Coord(coord.Z, coord.X, coord.Y)); - } - else if (stats.TargetUsed) - { - report.RecordMerged(); - } - else - { - report.RecordReplaced(); - } - tiles.Add(tile); currentBatchBytes += tile.Size(); @@ -210,10 +194,6 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal currentBatchBytes = 0; } } - else - { - report.RecordSkipped(); - } tileProgressCount++; overallTileProgressCount++; diff --git a/MergerServiceUnitTests/Models/MergeReportTest.cs b/MergerServiceUnitTests/Models/MergeReportTest.cs index 3cbc7a5..3063683 100644 --- a/MergerServiceUnitTests/Models/MergeReportTest.cs +++ b/MergerServiceUnitTests/Models/MergeReportTest.cs @@ -1,4 +1,5 @@ using MergerLogic.DataTypes; +using MergerLogic.ImageProcessing; using MergerService.Models.Reports; using Microsoft.VisualStudio.TestTools.UnitTesting; using Newtonsoft.Json.Linq; @@ -10,15 +11,75 @@ namespace MergerServiceUnitTests.Models [TestCategory("unit")] public class MergeReportTest { + private static readonly Coord AnyCoord = new Coord(10, 1, 2); + + [TestMethod] + public void RecordOutcome_TileNotProduced_CountsAsSkipped() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordOutcome(AnyCoord, existedBefore: true, tileProduced: false, new MergeStats(true, true)); + Assert.AreEqual(1, report.Skipped); + Assert.AreEqual(0, report.Added + report.Merged + report.Replaced); + } + + [TestMethod] + public void RecordOutcome_NoSourceData_CountsAsSkipped() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordOutcome(AnyCoord, existedBefore: false, tileProduced: true, new MergeStats(true, false)); + Assert.AreEqual(1, report.Skipped); + Assert.AreEqual(0, report.Added); + } + + [TestMethod] + public void RecordOutcome_NotExistedBefore_CountsAsAdded() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordOutcome(new Coord(10, 5, 6), existedBefore: false, tileProduced: true, new MergeStats(false, true)); + Assert.AreEqual(1, report.Added); + Assert.AreEqual(1, report.AddedTiles.Count); + Assert.AreEqual(new Coord(10, 5, 6), report.AddedTiles[0]); + } + + [TestMethod] + public void RecordOutcome_ExistedAndTargetBlended_CountsAsMerged() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordOutcome(AnyCoord, existedBefore: true, tileProduced: true, new MergeStats(true, true)); + Assert.AreEqual(1, report.Merged); + Assert.AreEqual(0, report.Added); + } + + [TestMethod] + public void RecordOutcome_ExistedAndOpaqueSource_CountsAsReplaced() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + report.RecordOutcome(AnyCoord, existedBefore: true, tileProduced: true, new MergeStats(false, true)); + Assert.AreEqual(1, report.Replaced); + Assert.AreEqual(0, report.Merged); + } + + [TestMethod] + public void RecordOutcome_StoresCopyOfAddedCoord() + { + var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); + var coord = new Coord(10, 7, 8); + report.RecordOutcome(coord, existedBefore: false, tileProduced: true, new MergeStats(false, true)); + + // mutating the caller's coord must not affect the stored one + coord.Y = 999; + Assert.AreEqual(8, report.AddedTiles[0].Y); + } + [TestMethod] public void Counts_And_Percentages_Are_Computed() { var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); - report.RecordAdded(new Coord(10, 1, 2)); - report.RecordAdded(new Coord(10, 1, 3)); - report.RecordMerged(); - report.RecordReplaced(); - report.RecordSkipped(); + report.RecordOutcome(new Coord(10, 1, 2), existedBefore: false, tileProduced: true, new MergeStats(false, true)); // added + report.RecordOutcome(new Coord(10, 1, 3), existedBefore: false, tileProduced: true, new MergeStats(false, true)); // added + report.RecordOutcome(AnyCoord, existedBefore: true, tileProduced: true, new MergeStats(true, true)); // merged + report.RecordOutcome(AnyCoord, existedBefore: true, tileProduced: true, new MergeStats(false, true)); // replaced + report.RecordOutcome(AnyCoord, existedBefore: false, tileProduced: false, new MergeStats(false, false)); // skipped report.Finalize(new DateTime(2026, 9, 15, 0, 0, 0, DateTimeKind.Utc), new DateTime(2026, 9, 15, 0, 1, 0, DateTimeKind.Utc)); @@ -36,7 +97,7 @@ public void Counts_And_Percentages_Are_Computed() public void Json_Includes_AddedTiles_LogString_Excludes_Them() { var report = new MergeReport("job1", "task1", "MERGE", "PNG", false); - report.RecordAdded(new Coord(10, 1, 2)); + report.RecordOutcome(new Coord(10, 1, 2), existedBefore: false, tileProduced: true, new MergeStats(false, true)); report.Finalize(DateTime.UnixEpoch, DateTime.UnixEpoch); JObject json = JObject.Parse(report.ToJson()); diff --git a/MergerServiceUnitTests/Runners/TaskExecutorTest.cs b/MergerServiceUnitTests/Runners/TaskExecutorTest.cs index 04b78f0..8e27d80 100644 --- a/MergerServiceUnitTests/Runners/TaskExecutorTest.cs +++ b/MergerServiceUnitTests/Runners/TaskExecutorTest.cs @@ -195,10 +195,13 @@ public void WhenConfiguringBatchLimits_ShouldWriteTilesEachTimeAfterReachingBatc )); } + // Integration/wiring test: verifies ExecuteTask feeds RecordOutcome the right inputs + // (existedBefore, tile produced, MergeStats) and writes the resulting report. + // The exhaustive added/merged/replaced/skipped classification permutations live in MergeReportTest. [TestMethod] [TestCategory("unit")] [TestCategory("runners")] - public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() + public void ExecuteTask_ClassifiesTileAndWritesReport() { this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); @@ -219,6 +222,7 @@ public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) ).Returns(sourceDataMock.Object); + // target already has this tile → existedBefore == true; merger reports target blended → merged targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(true); TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); @@ -226,7 +230,7 @@ public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); - MergeStats outStats = new MergeStats(true, true); + MergeStats outStats = new MergeStats(targetUsed: true, anySourceUsed: true); this._tileMergerMock.Setup(m => m.MergeTiles( It.IsAny>(), It.IsAny(), It.IsAny(), out outStats, It.IsAny()) @@ -242,6 +246,7 @@ public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); + this._reportWriterMock.Verify(w => w.WriteReport(It.IsAny(), "reports"), Times.Once); Assert.IsNotNull(captured); Assert.AreEqual(1, captured.Merged); Assert.AreEqual(0, captured.Added); @@ -249,223 +254,6 @@ public void ExecuteTask_ExistingTargetTileBlended_CountsAsMerged() Assert.AreEqual(0, captured.Skipped); } - [TestMethod] - [TestCategory("unit")] - [TestCategory("runners")] - public void ExecuteTask_NewTargetTile_CountsAsAdded() - { - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); - - byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); - Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); - Source testSource = new Source("source", "source_type"); - Mock targetDataMock = this._mockRepository.Create(); - Mock sourceDataMock = this._mockRepository.Create(); - - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testTarget.Type, testTarget.Path, It.IsAny(), - testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) - ).Returns(targetDataMock.Object); - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testSource.Type, testSource.Path, It.IsAny(), - testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) - ).Returns(sourceDataMock.Object); - - targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(false); - - TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); - var testTask = new MergeTask("id", "type", "description", - new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), - Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); - - MergeStats outStats = new MergeStats(false, true); - this._tileMergerMock.Setup(m => m.MergeTiles( - It.IsAny>(), It.IsAny(), - It.IsAny(), out outStats, It.IsAny()) - ).Returns(new Tile(new Coord(1, 1, 1), tileBytes)); - - MergeReport captured = null; - this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) - .Callback((r, p) => captured = r); - - var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, - _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, - _metricsProviderMock.Object, _reportWriterMock.Object); - - testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); - - Assert.IsNotNull(captured); - Assert.AreEqual(1, captured.Added); - Assert.AreEqual(1, captured.AddedTiles.Count); - Assert.AreEqual(1, captured.AddedTiles[0].Z); - Assert.AreEqual(1, captured.AddedTiles[0].X); - Assert.AreEqual(1, captured.AddedTiles[0].Y); - Assert.AreEqual(0, captured.Merged); - Assert.AreEqual(0, captured.Replaced); - } - - [TestMethod] - [TestCategory("unit")] - [TestCategory("runners")] - public void ExecuteTask_OpaqueSourceOverExisting_CountsAsReplaced() - { - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); - - byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); - Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); - Source testSource = new Source("source", "source_type"); - Mock targetDataMock = this._mockRepository.Create(); - Mock sourceDataMock = this._mockRepository.Create(); - - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testTarget.Type, testTarget.Path, It.IsAny(), - testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) - ).Returns(targetDataMock.Object); - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testSource.Type, testSource.Path, It.IsAny(), - testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) - ).Returns(sourceDataMock.Object); - - targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(true); - - TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); - var testTask = new MergeTask("id", "type", "description", - new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), - Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); - - MergeStats outStats = new MergeStats(false, true); - this._tileMergerMock.Setup(m => m.MergeTiles( - It.IsAny>(), It.IsAny(), - It.IsAny(), out outStats, It.IsAny()) - ).Returns(new Tile(new Coord(1, 1, 1), tileBytes)); - - MergeReport captured = null; - this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) - .Callback((r, p) => captured = r); - - var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, - _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, - _metricsProviderMock.Object, _reportWriterMock.Object); - - testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); - - Assert.IsNotNull(captured); - Assert.AreEqual(1, captured.Replaced); - Assert.AreEqual(0, captured.Added); - Assert.AreEqual(0, captured.Merged); - } - - [TestMethod] - [TestCategory("unit")] - [TestCategory("runners")] - public void ExecuteTask_MergeReturnsNull_CountsAsSkipped() - { - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); - - Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); - Source testSource = new Source("source", "source_type"); - Mock targetDataMock = this._mockRepository.Create(); - Mock sourceDataMock = this._mockRepository.Create(); - - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testTarget.Type, testTarget.Path, It.IsAny(), - testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) - ).Returns(targetDataMock.Object); - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testSource.Type, testSource.Path, It.IsAny(), - testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) - ).Returns(sourceDataMock.Object); - - targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(false); - - TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); - var testTask = new MergeTask("id", "type", "description", - new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), - Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); - - MergeStats outStats = new MergeStats(false, false); - this._tileMergerMock.Setup(m => m.MergeTiles( - It.IsAny>(), It.IsAny(), - It.IsAny(), out outStats, It.IsAny()) - ).Returns((Tile)null); - - MergeReport captured = null; - this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) - .Callback((r, p) => captured = r); - - var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, - _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, - _metricsProviderMock.Object, _reportWriterMock.Object); - - testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); - - Assert.IsNotNull(captured); - Assert.AreEqual(1, captured.Skipped); - Assert.AreEqual(0, captured.Added); - Assert.AreEqual(0, captured.Merged); - Assert.AreEqual(0, captured.Replaced); - } - - [TestMethod] - [TestCategory("unit")] - [TestCategory("runners")] - public void ExecuteTask_TileWithNoSourceData_CountsAsSkipped() - { - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "batchMaxSize")).Returns(1); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchSize", "limitBatchSize")).Returns(true); - this._configurationManagerMock.Setup(configManager => configManager.GetConfiguration("GENERAL", "batchMaxBytes")).Returns(1); - - byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); - Source testTarget = new Source("target", "target_type", new Extent(), GridOrigin.UPPER_LEFT, Grid.TwoXOne); - Source testSource = new Source("source", "source_type"); - Mock targetDataMock = this._mockRepository.Create(); - Mock sourceDataMock = this._mockRepository.Create(); - - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testTarget.Type, testTarget.Path, It.IsAny(), - testTarget.Grid, testTarget.Origin, testTarget.Extent, It.IsAny()) - ).Returns(targetDataMock.Object); - this._dataFactoryMock.Setup(dataFactory => dataFactory.CreateDataSource( - testSource.Type, testSource.Path, It.IsAny(), - testSource.Grid, testSource.Origin, testSource.Extent, It.IsAny()) - ).Returns(sourceDataMock.Object); - - targetDataMock.Setup(targetData => targetData.TileExists(It.IsAny())).Returns(false); - - TileBounds tileBounds = new TileBounds(1, 1, 1, 1, 1); - var testTask = new MergeTask("id", "type", "description", - new MergeMetadata(TileFormat.Jpeg, false, new TileBounds[] { tileBounds }, new Source[] { testTarget, testSource }), - Status.PENDING, null, "reason", 0, "jobId", true, new DateTime(), new DateTime()); - - MergeStats outStats = new MergeStats(targetUsed: true, anySourceUsed: false); - this._tileMergerMock.Setup(m => m.MergeTiles( - It.IsAny>(), It.IsAny(), - It.IsAny(), out outStats, It.IsAny()) - ).Returns(new Tile(new Coord(1, 1, 1), tileBytes)); - - MergeReport captured = null; - this._reportWriterMock.Setup(w => w.WriteReport(It.IsAny(), "reports")) - .Callback((r, p) => captured = r); - - var testTaskExecutor = new TaskExecutor(_dataFactoryMock.Object, _tileMergerMock.Object, _timeUtilsMock.Object, - _configurationManagerMock.Object, _taskExecutorLoggerMock.Object, _testActivitySource, _testFileSystem, - _metricsProviderMock.Object, _reportWriterMock.Object); - - testTaskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, null, "reports"); - - Assert.IsNotNull(captured); - Assert.AreEqual(1, captured.Skipped); - Assert.AreEqual(0, captured.Added); - Assert.AreEqual(0, captured.Merged); - Assert.AreEqual(0, captured.Replaced); - } - private Tuple, Tile[]> SetupTestTask(int amountOfSources, bool isTargetNew) { byte[] tileBytes = File.ReadAllBytes("tile.jpeg"); From 95d69d57e4afac9bce2ded0d1212ec07e3f7fa98 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 17:49:31 +0300 Subject: [PATCH 18/27] feat(logging): structured JSON logs with jobId/taskId scope for correlation Emit each log line as a single JSON object instead of bracketed text, and wrap task handling in TaskRunner.RunTask in an ILogger.BeginScope carrying jobId/taskId (IncludeScopes enabled). The console exporter flattens scope pairs to top-level fields, so all of a task's logs are correlatable per job/task and parseable downstream via a Loki `| json` stage (no regex). Log line shape: {"time":..,"level":..,"service":"gpkg-merger","version":..,"category":.., "thread":..,"message":..,"jobId":..,"taskId":..} Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Extensions/ServiceCollectionExtensions.cs | 2 + .../OpenTelemetryFormattedConsoleExporter.cs | 42 ++++++- ...enTelemetryFormattedConsoleExporterTest.cs | 92 +++++++++++++++ MergerService/Runners/TaskRunner.cs | 108 +++++++++--------- .../Runners/TaskRunnerTest.cs | 25 ++++ 5 files changed, 214 insertions(+), 55 deletions(-) create mode 100644 MergerLogicUnitTests/Monitoring/OpenTelemetryFormattedConsoleExporterTest.cs diff --git a/MergerLogic/Extensions/ServiceCollectionExtensions.cs b/MergerLogic/Extensions/ServiceCollectionExtensions.cs index e3eeda9..087e052 100644 --- a/MergerLogic/Extensions/ServiceCollectionExtensions.cs +++ b/MergerLogic/Extensions/ServiceCollectionExtensions.cs @@ -152,6 +152,8 @@ public static IServiceCollection RegisterOpenTelemetry(this IServiceCollection c options.AddProcessor( new SimpleLogRecordExportProcessor(new OpenTelemetryFormattedConsoleExporter(new ConsoleExporterOptions()))); //lgtm [cs/local-not-disposed] options.SetResourceBuilder(resourceBuilder); + // required for the exporter to emit BeginScope correlation fields (jobId/taskId) + options.IncludeScopes = true; }); }); #endregion Logger diff --git a/MergerLogic/Monitoring/OpenTelemetryFormattedConsoleExporter.cs b/MergerLogic/Monitoring/OpenTelemetryFormattedConsoleExporter.cs index 45c56f9..908cc4b 100644 --- a/MergerLogic/Monitoring/OpenTelemetryFormattedConsoleExporter.cs +++ b/MergerLogic/Monitoring/OpenTelemetryFormattedConsoleExporter.cs @@ -1,6 +1,7 @@ -using OpenTelemetry; +using OpenTelemetry; using OpenTelemetry.Exporter; using OpenTelemetry.Logs; +using System.Text.Json; namespace MergerLogic.Monitoring { @@ -29,9 +30,44 @@ private string MCTextFormat(LogRecord record) var resource = this.ParseResource(); var serviceName = this.GetResourceAttribute(resource, SERVICE_NAME_ATTRIBUTE, "unknown_service"); var serviceVersion = this.GetResourceAttribute(resource, SERVICE_VERSION_ATTRIBUTE, "unknown_version"); - var exception = record.Exception != null ? $" [{record.Exception}]" : string.Empty; - return $"[{this.FormatTime(record.Timestamp)}] [{record.LogLevel}] [{serviceName}] [{serviceVersion}] [{record.CategoryName}] [{Environment.CurrentManagedThreadId}] {record.State}{exception}"; + var entry = new Dictionary + { + ["time"] = this.FormatTime(record.Timestamp), + ["level"] = record.LogLevel.ToString(), + ["service"] = serviceName, + ["version"] = serviceVersion, + ["category"] = record.CategoryName, + ["thread"] = Environment.CurrentManagedThreadId, + ["message"] = record.State?.ToString(), + }; + + if (record.Exception != null) + { + entry["exception"] = record.Exception.ToString(); + } + + this.AddScopes(record, entry); + + return JsonSerializer.Serialize(entry); + } + + // Flatten ILogger.BeginScope key/value pairs to top-level fields (e.g. jobId/taskId) so they + // are queryable in Loki. Empty unless IncludeScopes is enabled (see DI setup). + private void AddScopes(LogRecord record, Dictionary entry) + { + record.ForEachScope((scope, state) => + { + foreach (var pair in scope) + { + if (pair.Key == "{OriginalFormat}") + { + continue; + } + + state[pair.Key] = pair.Value; + } + }, entry); } private string FormatTime(DateTime time) diff --git a/MergerLogicUnitTests/Monitoring/OpenTelemetryFormattedConsoleExporterTest.cs b/MergerLogicUnitTests/Monitoring/OpenTelemetryFormattedConsoleExporterTest.cs new file mode 100644 index 0000000..9336b2c --- /dev/null +++ b/MergerLogicUnitTests/Monitoring/OpenTelemetryFormattedConsoleExporterTest.cs @@ -0,0 +1,92 @@ +using MergerLogic.Monitoring; +using Microsoft.Extensions.Logging; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using OpenTelemetry; +using OpenTelemetry.Exporter; +using OpenTelemetry.Logs; +using System; +using System.Collections.Generic; +using System.IO; +using System.Text.Json; + +namespace MergerLogicUnitTests.Monitoring +{ + [TestClass] + [TestCategory("unit")] + [TestCategory("monitoring")] + public class OpenTelemetryFormattedConsoleExporterTest + { + private TextWriter _originalOut = null!; + + [TestInitialize] + public void BeforeEach() + { + this._originalOut = Console.Out; + } + + [TestCleanup] + public void AfterEach() + { + Console.SetOut(this._originalOut); + } + + private static ILoggerFactory BuildFactory() + { + return LoggerFactory.Create(builder => + { + builder.ClearProviders(); + builder.AddOpenTelemetry(options => + { + options.IncludeScopes = true; + options.AddProcessor(new SimpleLogRecordExportProcessor( + new OpenTelemetryFormattedConsoleExporter(new ConsoleExporterOptions()))); + }); + }); + } + + private static JsonElement CaptureSingleLine(Action log) + { + var writer = new StringWriter(); + Console.SetOut(writer); + + using (var factory = BuildFactory()) + { + log(factory.CreateLogger("TestCategory")); + } + + string line = writer.ToString().Trim(); + return JsonDocument.Parse(line).RootElement; + } + + [TestMethod] + public void WhenLoggingInsideAScope_ShouldEmitScopeAsTopLevelJsonFields() + { + JsonElement entry = CaptureSingleLine(logger => + { + using (logger.BeginScope(new Dictionary + { + ["jobId"] = "job-123", + ["taskId"] = "task-456" + })) + { + logger.LogInformation("processing tiles"); + } + }); + + Assert.AreEqual("processing tiles", entry.GetProperty("message").GetString()); + Assert.AreEqual("job-123", entry.GetProperty("jobId").GetString()); + Assert.AreEqual("task-456", entry.GetProperty("taskId").GetString()); + Assert.AreEqual("Information", entry.GetProperty("level").GetString()); + } + + [TestMethod] + public void WhenLoggingWithoutAScope_ShouldNotEmitScopeFields() + { + JsonElement entry = CaptureSingleLine(logger => logger.LogInformation("no scope here")); + + Assert.AreEqual("no scope here", entry.GetProperty("message").GetString()); + Assert.IsFalse(entry.TryGetProperty("jobId", out _), "unexpected jobId field"); + Assert.IsFalse(entry.TryGetProperty("taskId", out _), "unexpected taskId field"); + } + } +} diff --git a/MergerService/Runners/TaskRunner.cs b/MergerService/Runners/TaskRunner.cs index 0088416..b9d9c58 100644 --- a/MergerService/Runners/TaskRunner.cs +++ b/MergerService/Runners/TaskRunner.cs @@ -86,73 +86,77 @@ public bool RunTask(MergeTask? task) return false; } - this._logger.LogInformation($"[{methodName}] Run Task: jobId {task.JobId}, taskId {task.Id}"); - string? managerCallbackUrl = this._jobUtils.GetJob(task.JobId)?.Parameters.AdditionalParams?.JobTrackerServiceURL; - string log = managerCallbackUrl == null ? "managerCallbackUrl not provided as job parameter" : $"managerCallback url: {managerCallbackUrl}"; - this._logger.LogDebug($"[{methodName}]{log}"); - - // check if needs to fail task that was released by task liberator and reached max attempts - if (task.Attempts >= this._maxTaskRetriesAttempts) + // tag every log line from this task with jobId/taskId for per-task correlation + using (this._logger.BeginScope(new Dictionary { ["jobId"] = task.JobId, ["taskId"] = task.Id })) { + this._logger.LogInformation($"[{methodName}] Run Task: jobId {task.JobId}, taskId {task.Id}"); + string? managerCallbackUrl = this._jobUtils.GetJob(task.JobId)?.Parameters.AdditionalParams?.JobTrackerServiceURL; + string log = managerCallbackUrl == null ? "managerCallbackUrl not provided as job parameter" : $"managerCallback url: {managerCallbackUrl}"; + this._logger.LogDebug($"[{methodName}]{log}"); + + // check if needs to fail task that was released by task liberator and reached max attempts + if (task.Attempts >= this._maxTaskRetriesAttempts) + { + try + { + string reason = string.IsNullOrEmpty(task.Reason) ? $"Max attempts reached, current attempt is {task.Attempts}" : $"{task.Reason} and Max attempts reached with {task.Attempts} attempts"; + this._logger.LogWarning($"[{methodName}] reject job because attemts count reached, jobId {task.JobId}, taskId {task.Id}, {reason}"); + this._taskUtils.UpdateReject(task.JobId, task.Id, task.Attempts, reason, task.Resettable, managerCallbackUrl); + } + catch (Exception innerError) + { + this._logger.LogError(innerError, $"[{methodName}] Error in MergerService while updating reject status for job {task.JobId}, task {task.Id} due to max attemps reached with {task.Attempts}, update task failure: {innerError.Message}"); + } + + return false; + } + + var totalTaskStopwatch = Stopwatch.StartNew(); + bool taskSucceed = false; + try { - string reason = string.IsNullOrEmpty(task.Reason) ? $"Max attempts reached, current attempt is {task.Attempts}" : $"{task.Reason} and Max attempts reached with {task.Attempts} attempts"; - this._logger.LogWarning($"[{methodName}] reject job because attemts count reached, jobId {task.JobId}, taskId {task.Id}, {reason}"); - this._taskUtils.UpdateReject(task.JobId, task.Id, task.Attempts, reason, task.Resettable, managerCallbackUrl); + this._heartbeatClient.Start(task.Id); + this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl); + taskSucceed = true; } - catch (Exception innerError) + catch (Exception e) { - this._logger.LogError(innerError, $"[{methodName}] Error in MergerService while updating reject status for job {task.JobId}, task {task.Id} due to max attemps reached with {task.Attempts}, update task failure: {innerError.Message}"); + this._logger.LogError(e, $"[{methodName}] Error in MergerService while running task {task.Id}, error: {e.Message}"); + + try + { + this._taskUtils.UpdateReject(task.JobId, task.Id, task.Attempts, e.Message, true, managerCallbackUrl); + } + catch (Exception innerError) + { + this._logger.LogError(e, $"[{methodName}] Error in MergerService while updating reject status, RunTask catch block - update task failure: {innerError.Message}"); + } + } + finally + { + totalTaskStopwatch.Stop(); + this._metricsProvider.TaskExecutionTimeHistogram(totalTaskStopwatch.Elapsed.TotalSeconds, task.Type); + this._heartbeatClient.Stop(); } - return false; - } - - var totalTaskStopwatch = Stopwatch.StartNew(); - bool taskSucceed = false; - - try - { - this._heartbeatClient.Start(task.Id); - this._taskExecutor.ExecuteTask(task, this._taskUtils, managerCallbackUrl); - taskSucceed = true; - } - catch (Exception e) - { - this._logger.LogError(e, $"[{methodName}] Error in MergerService while running task {task.Id}, error: {e.Message}"); + if (!taskSucceed) + { + return false; + } try { - this._taskUtils.UpdateReject(task.JobId, task.Id, task.Attempts, e.Message, true, managerCallbackUrl); + this._taskUtils.UpdateCompletion(task.JobId, task.Id, managerCallbackUrl); + this._logger.LogInformation($"[{methodName}] Completed task: jobId: {task.JobId}, taskId: {task.Id}"); } - catch (Exception innerError) + catch (Exception e) { - this._logger.LogError(e, $"[{methodName}] Error in MergerService while updating reject status, RunTask catch block - update task failure: {innerError.Message}"); + this._logger.LogError(e, $"[{methodName}] Error in MergerService start - update task completion: {e.Message}"); } - } - finally - { - totalTaskStopwatch.Stop(); - this._metricsProvider.TaskExecutionTimeHistogram(totalTaskStopwatch.Elapsed.TotalSeconds, task.Type); - this._heartbeatClient.Stop(); - } - - if (!taskSucceed) - { - return false; - } - try - { - this._taskUtils.UpdateCompletion(task.JobId, task.Id, managerCallbackUrl); - this._logger.LogInformation($"[{methodName}] Completed task: jobId: {task.JobId}, taskId: {task.Id}"); + return true; } - catch (Exception e) - { - this._logger.LogError(e, $"[{methodName}] Error in MergerService start - update task completion: {e.Message}"); - } - - return true; } } } diff --git a/MergerServiceUnitTests/Runners/TaskRunnerTest.cs b/MergerServiceUnitTests/Runners/TaskRunnerTest.cs index c518ee4..c70fc39 100644 --- a/MergerServiceUnitTests/Runners/TaskRunnerTest.cs +++ b/MergerServiceUnitTests/Runners/TaskRunnerTest.cs @@ -86,6 +86,31 @@ public void WhenTaskExecutedSuccessfully_ShouldUpdateTaskCompletion() _taskUtilsMock.Verify(taskUtils => taskUtils.UpdateCompletion(testTask.JobId, testTask.Id, It.IsAny()), Times.Once); } + [TestMethod] + public void WhenRunningTask_ShouldScopeLogsWithJobAndTaskId() + { + var testTask = new MergeTask("testTaskId", "type", "description", + new MergeMetadata(TileFormat.Jpeg, true, new TileBounds[0], new Source[0]), + Status.PENDING, 0, "reason", 0, "testJobId", true, new DateTime(), new DateTime()); + + Dictionary? capturedScope = null; + this._taskUtilsMock.Setup(taskUtils => taskUtils.GetTask(It.IsAny(), It.IsAny())).Returns(testTask); + this._taskExecutorMock.Setup(taskExecutor => taskExecutor.ExecuteTask(testTask, _taskUtilsMock.Object, It.IsAny())); + this._loggerMock.Setup(logger => logger.BeginScope(It.IsAny>())) + .Returns(Mock.Of()) + .Callback>(scope => capturedScope = scope); + + var testTaskRunner = new TaskRunner(_taskExecutorMock.Object, _jobUtilsMock.Object, _loggerMock.Object, + _taskUtilsMock.Object, _heartbeatClientMock.Object, _metricsProviderMock.Object, + _configurationManagerMock.Object); + + testTaskRunner.RunTask(testTaskRunner.FetchTask(new KeyValuePair("testJobType", "testTaskType"))); + + Assert.IsNotNull(capturedScope); + Assert.AreEqual(testTask.JobId, capturedScope["jobId"]); + Assert.AreEqual(testTask.Id, capturedScope["taskId"]); + } + [TestMethod] public void WhenTaskExecutionFailed_ShouldUpdateTaskFailed() { From 69fac1d875189b452854487bc3a997531b95c28c Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 17:59:09 +0300 Subject: [PATCH 19/27] feat(dashboard): add per-task log investigation dashboard New MergerLogsDashboard.json for job/task log drill-down over the structured JSON logs. namespace dropdown + free-text jobId/taskId vars filter a logs panel, an errors/warnings panel, a per-level log-rate graph, a line-count stat, and a parsed timeline table. Queries parse via `| json` and drop non-JSON lines with `| __error__=""`; once MAPCO-11775 attaches jobId/taskId as Loki structured metadata the same filters resolve without full-line parse. Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 360 +++++++++++++++++++++ 1 file changed, 360 insertions(+) create mode 100644 Assets/Dashboards/MergerLogsDashboard.json diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json new file mode 100644 index 0000000..d288e17 --- /dev/null +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -0,0 +1,360 @@ +{ + "title": "GPKG-Merger \u2014 Logs", + "uid": "gpkg-merger-logs", + "tags": [ + "gpkg-merger", + "logs" + ], + "timezone": "", + "schemaVersion": 39, + "version": 1, + "editable": true, + "refresh": "30s", + "time": { + "from": "now-3h", + "to": "now" + }, + "templating": { + "list": [ + { + "name": "loki", + "label": "Loki datasource", + "type": "datasource", + "query": "loki", + "current": {}, + "hide": 0, + "includeAll": false, + "multi": false, + "refresh": 1, + "regex": "", + "skipUrlSync": false + }, + { + "name": "namespace", + "label": "namespace", + "type": "query", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "query": { + "label": "k8s_namespace_name", + "stream": "{k8s_deployment_name=~\"gpkg-merger.*\"}", + "type": 4, + "refId": "LokiVariableQueryEditor-VariableQuery" + }, + "definition": "label_values({k8s_deployment_name=~\"gpkg-merger.*\"}, k8s_namespace_name)", + "current": { + "text": "All", + "value": "$__all", + "selected": true + }, + "includeAll": true, + "multi": true, + "refresh": 2, + "regex": "", + "sort": 1, + "hide": 0, + "skipUrlSync": false + }, + { + "name": "job", + "label": "jobId", + "type": "textbox", + "query": "", + "current": { + "text": "", + "value": "" + }, + "hide": 0, + "skipUrlSync": false + }, + { + "name": "task", + "label": "taskId", + "type": "textbox", + "query": "", + "current": { + "text": "", + "value": "" + }, + "hide": 0, + "skipUrlSync": false + } + ] + }, + "panels": [ + { + "id": 1, + "type": "row", + "title": "Per-Task Log Investigation", + "collapsed": false, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 0 + }, + "panels": [] + }, + { + "id": 2, + "type": "logs", + "title": "Task logs", + "description": "All logs for the selected job/task. Leave jobId/taskId empty to see everything; paste an id to drill down.", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 12, + "w": 16, + "x": 0, + "y": 1 + }, + "options": { + "showTime": true, + "showLabels": false, + "showCommonLabels": false, + "wrapLogMessage": true, + "prettifyLogMessage": true, + "enableLogDetails": true, + "dedupStrategy": "none", + "sortOrder": "Descending" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\"", + "queryType": "range" + } + ] + }, + { + "id": 3, + "type": "timeseries", + "title": "Log rate by level", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 6, + "w": 8, + "x": 16, + "y": 1 + }, + "fieldConfig": { + "defaults": { + "custom": { + "drawStyle": "bars", + "fillOpacity": 40, + "stacking": { + "mode": "normal" + } + } + }, + "overrides": [] + }, + "options": { + "legend": { + "displayMode": "list", + "placement": "bottom", + "showLegend": true + } + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum by (level) (count_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" [$__interval]))", + "queryType": "range", + "legendFormat": "{{level}}" + } + ] + }, + { + "id": 4, + "type": "stat", + "title": "Log lines (range)", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 6, + "w": 8, + "x": 16, + "y": 7 + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "green", + "value": null + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(count_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 5, + "type": "logs", + "title": "Errors & warnings", + "description": "Error/Warning/Critical lines for the current filter.", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 8, + "w": 24, + "x": 0, + "y": 13 + }, + "options": { + "showTime": true, + "showLabels": false, + "showCommonLabels": false, + "wrapLogMessage": true, + "prettifyLogMessage": true, + "enableLogDetails": true, + "dedupStrategy": "none", + "sortOrder": "Descending" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | level=~\"Error|Warning|Critical\"", + "queryType": "range" + } + ] + }, + { + "id": 6, + "type": "table", + "title": "Log timeline (parsed)", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 10, + "w": 24, + "x": 0, + "y": 21 + }, + "fieldConfig": { + "defaults": { + "custom": { + "filterable": true, + "align": "auto" + } + }, + "overrides": [] + }, + "options": { + "showHeader": true, + "cellHeight": "sm", + "footer": { + "show": false + } + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\"", + "queryType": "range" + } + ], + "transformations": [ + { + "id": "organize", + "options": { + "excludeByName": { + "service": true, + "version": true, + "thread": true, + "jobId": true, + "taskId": true, + "service_name": true, + "k8s_container_name": true, + "k8s_deployment_name": true, + "k8s_namespace_name": true, + "k8s_pod_name": true, + "time": true, + "Line": true, + "id": true, + "tsNs": true, + "labels": true + }, + "indexByName": { + "Time": 0, + "level": 1, + "category": 2, + "message": 3 + }, + "renameByName": { + "level": "level", + "category": "category", + "message": "message" + } + } + } + ] + } + ], + "annotations": { + "list": [] + } +} \ No newline at end of file From e9320689806a67c2707e6d3edaada5e59b5ecbe3 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 18:42:21 +0300 Subject: [PATCH 20/27] feat: per-task/per-job statistics from logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Emit the merge-report counts (added/merged/replaced/skipped/total/durationSeconds) as structured scope fields so they land as top-level fields in the JSON log line, then aggregate them in Loki per job/task — no Prometheus cardinality cost. Extends the logs dashboard with a stats section: per-filter stat tiles (counts + p95 duration), a tiles-by-outcome throughput graph, a duration p50/p95 graph, and a per-task merge-report table. Driven by the existing $namespace/$job/$task vars. Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 577 +++++++++++++++++++++ MergerService/Runners/TaskExecutor.cs | 15 +- 2 files changed, 591 insertions(+), 1 deletion(-) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index d288e17..ced6316 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -352,6 +352,583 @@ } } ] + }, + { + "id": 7, + "type": "row", + "title": "Per-Task / Per-Job Statistics", + "collapsed": false, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 31 + }, + "panels": [] + }, + { + "id": 8, + "type": "stat", + "title": "Added", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 5, + "w": 4, + "x": 0, + "y": 32 + }, + "fieldConfig": { + "defaults": { + "unit": "short", + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "blue", + "value": null + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap added [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 9, + "type": "stat", + "title": "Merged", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 5, + "w": 4, + "x": 4, + "y": 32 + }, + "fieldConfig": { + "defaults": { + "unit": "short", + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "blue", + "value": null + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap merged [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 10, + "type": "stat", + "title": "Replaced", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 5, + "w": 4, + "x": 8, + "y": 32 + }, + "fieldConfig": { + "defaults": { + "unit": "short", + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "blue", + "value": null + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap replaced [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 11, + "type": "stat", + "title": "Skipped", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 5, + "w": 4, + "x": 12, + "y": 32 + }, + "fieldConfig": { + "defaults": { + "unit": "short", + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "blue", + "value": null + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap skipped [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 12, + "type": "stat", + "title": "Total tiles", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 5, + "w": 4, + "x": 16, + "y": 32 + }, + "fieldConfig": { + "defaults": { + "unit": "short", + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "blue", + "value": null + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap total [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 13, + "type": "stat", + "title": "p95 duration", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 5, + "w": 4, + "x": 20, + "y": 32 + }, + "fieldConfig": { + "defaults": { + "unit": "s", + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "green", + "value": null + }, + { + "color": "orange", + "value": 60 + }, + { + "color": "red", + "value": 120 + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "max(quantile_over_time(0.95, {k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap durationSeconds [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 14, + "type": "timeseries", + "title": "Tiles by outcome (per $__interval)", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 0, + "y": 37 + }, + "fieldConfig": { + "defaults": { + "custom": { + "drawStyle": "bars", + "fillOpacity": 50, + "stacking": { + "mode": "normal" + } + } + }, + "overrides": [] + }, + "options": { + "legend": { + "displayMode": "list", + "placement": "bottom", + "showLegend": true + } + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(sum_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap added [$__interval]))", + "queryType": "range", + "legendFormat": "added" + }, + { + "refId": "B", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(sum_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap merged [$__interval]))", + "queryType": "range", + "legendFormat": "merged" + }, + { + "refId": "C", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(sum_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap replaced [$__interval]))", + "queryType": "range", + "legendFormat": "replaced" + }, + { + "refId": "D", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(sum_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap skipped [$__interval]))", + "queryType": "range", + "legendFormat": "skipped" + } + ] + }, + { + "id": 15, + "type": "timeseries", + "title": "Task duration (s)", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 12, + "y": 37 + }, + "fieldConfig": { + "defaults": { + "unit": "s", + "custom": { + "drawStyle": "line", + "fillOpacity": 10 + } + }, + "overrides": [] + }, + "options": { + "legend": { + "displayMode": "list", + "placement": "bottom", + "showLegend": true + } + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "max(quantile_over_time(0.5, {k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap durationSeconds [$__interval]))", + "queryType": "range", + "legendFormat": "p50" + }, + { + "refId": "B", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "max(quantile_over_time(0.95, {k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap durationSeconds [$__interval]))", + "queryType": "range", + "legendFormat": "p95" + } + ] + }, + { + "id": 16, + "type": "table", + "title": "Per-task merge reports", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 10, + "w": 24, + "x": 0, + "y": 45 + }, + "fieldConfig": { + "defaults": { + "custom": { + "filterable": true, + "align": "auto" + } + }, + "overrides": [] + }, + "options": { + "showHeader": true, + "cellHeight": "sm", + "footer": { + "show": false + } + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\"", + "queryType": "range" + } + ], + "transformations": [ + { + "id": "organize", + "options": { + "excludeByName": { + "service": true, + "version": true, + "thread": true, + "service_name": true, + "k8s_container_name": true, + "k8s_deployment_name": true, + "k8s_namespace_name": true, + "k8s_pod_name": true, + "time": true, + "Line": true, + "id": true, + "tsNs": true, + "labels": true, + "level": true, + "category": true, + "message": true + }, + "indexByName": { + "Time": 0, + "jobId": 1, + "taskId": 2, + "added": 3, + "merged": 4, + "replaced": 5, + "skipped": 6, + "total": 7, + "durationSeconds": 8 + }, + "renameByName": { + "jobId": "job", + "taskId": "task", + "durationSeconds": "duration(s)" + } + } + } + ] } ], "annotations": { diff --git a/MergerService/Runners/TaskExecutor.cs b/MergerService/Runners/TaskExecutor.cs index 85a6240..e71f854 100644 --- a/MergerService/Runners/TaskExecutor.cs +++ b/MergerService/Runners/TaskExecutor.cs @@ -267,7 +267,20 @@ public void ExecuteTask(MergeTask task, ITaskUtils taskUtils, string? managerCal } report.Finalize(reportStart, DateTime.UtcNow); - this._logger.LogInformation($"[{methodName}] Merge report: {report.ToLogString()}"); + // emit the report counts as structured scope fields (top-level in the JSON log line) + // so per-job/task statistics can be aggregated straight from the logs + using (this._logger.BeginScope(new Dictionary + { + ["added"] = report.Added, + ["merged"] = report.Merged, + ["replaced"] = report.Replaced, + ["skipped"] = report.Skipped, + ["total"] = report.Total, + ["durationSeconds"] = report.DurationSeconds, + })) + { + this._logger.LogInformation($"[{methodName}] Merge report: {report.ToLogString()}"); + } try { this._reportWriter.WriteReport(report, reportOutputPath); From cc62497569cbb970d594533b537ad3db66a618d8 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 20:37:38 +0300 Subject: [PATCH 21/27] feat(dashboard): metrics-first logs dashboard with clickable task drill-down Drop the row grouping and hoist the key stats (added/merged/replaced/skipped/ total, p95 duration) plus throughput/duration graphs to the top; logs move below. The per-task table's job/task cells are now data links that set the $job/$task variables, so you can pick a job/task from the in-range list (Loki can't offer real dropdowns for non-indexed fields). Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 651 +++++++++++---------- 1 file changed, 331 insertions(+), 320 deletions(-) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index ced6316..013273b 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -86,118 +86,21 @@ "panels": [ { "id": 1, - "type": "row", - "title": "Per-Task Log Investigation", - "collapsed": false, - "gridPos": { - "h": 1, - "w": 24, - "x": 0, - "y": 0 - }, - "panels": [] - }, - { - "id": 2, - "type": "logs", - "title": "Task logs", - "description": "All logs for the selected job/task. Leave jobId/taskId empty to see everything; paste an id to drill down.", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "gridPos": { - "h": 12, - "w": 16, - "x": 0, - "y": 1 - }, - "options": { - "showTime": true, - "showLabels": false, - "showCommonLabels": false, - "wrapLogMessage": true, - "prettifyLogMessage": true, - "enableLogDetails": true, - "dedupStrategy": "none", - "sortOrder": "Descending" - }, - "targets": [ - { - "refId": "A", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "editorMode": "code", - "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\"", - "queryType": "range" - } - ] - }, - { - "id": 3, - "type": "timeseries", - "title": "Log rate by level", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "gridPos": { - "h": 6, - "w": 8, - "x": 16, - "y": 1 - }, - "fieldConfig": { - "defaults": { - "custom": { - "drawStyle": "bars", - "fillOpacity": 40, - "stacking": { - "mode": "normal" - } - } - }, - "overrides": [] - }, - "options": { - "legend": { - "displayMode": "list", - "placement": "bottom", - "showLegend": true - } - }, - "targets": [ - { - "refId": "A", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "editorMode": "code", - "expr": "sum by (level) (count_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" [$__interval]))", - "queryType": "range", - "legendFormat": "{{level}}" - } - ] - }, - { - "id": 4, "type": "stat", - "title": "Log lines (range)", + "title": "Added", "datasource": { "type": "loki", "uid": "${loki}" }, "gridPos": { - "h": 6, - "w": 8, - "x": 16, - "y": 7 + "h": 5, + "w": 4, + "x": 0, + "y": 0 }, "fieldConfig": { "defaults": { + "unit": "short", "color": { "mode": "thresholds" }, @@ -205,7 +108,7 @@ "mode": "absolute", "steps": [ { - "color": "green", + "color": "blue", "value": null } ] @@ -232,144 +135,15 @@ "uid": "${loki}" }, "editorMode": "code", - "expr": "sum(count_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" [$__range]))", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap added [$__range]))", "queryType": "instant" } ] }, { - "id": 5, - "type": "logs", - "title": "Errors & warnings", - "description": "Error/Warning/Critical lines for the current filter.", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "gridPos": { - "h": 8, - "w": 24, - "x": 0, - "y": 13 - }, - "options": { - "showTime": true, - "showLabels": false, - "showCommonLabels": false, - "wrapLogMessage": true, - "prettifyLogMessage": true, - "enableLogDetails": true, - "dedupStrategy": "none", - "sortOrder": "Descending" - }, - "targets": [ - { - "refId": "A", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "editorMode": "code", - "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | level=~\"Error|Warning|Critical\"", - "queryType": "range" - } - ] - }, - { - "id": 6, - "type": "table", - "title": "Log timeline (parsed)", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "gridPos": { - "h": 10, - "w": 24, - "x": 0, - "y": 21 - }, - "fieldConfig": { - "defaults": { - "custom": { - "filterable": true, - "align": "auto" - } - }, - "overrides": [] - }, - "options": { - "showHeader": true, - "cellHeight": "sm", - "footer": { - "show": false - } - }, - "targets": [ - { - "refId": "A", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "editorMode": "code", - "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\"", - "queryType": "range" - } - ], - "transformations": [ - { - "id": "organize", - "options": { - "excludeByName": { - "service": true, - "version": true, - "thread": true, - "jobId": true, - "taskId": true, - "service_name": true, - "k8s_container_name": true, - "k8s_deployment_name": true, - "k8s_namespace_name": true, - "k8s_pod_name": true, - "time": true, - "Line": true, - "id": true, - "tsNs": true, - "labels": true - }, - "indexByName": { - "Time": 0, - "level": 1, - "category": 2, - "message": 3 - }, - "renameByName": { - "level": "level", - "category": "category", - "message": "message" - } - } - } - ] - }, - { - "id": 7, - "type": "row", - "title": "Per-Task / Per-Job Statistics", - "collapsed": false, - "gridPos": { - "h": 1, - "w": 24, - "x": 0, - "y": 31 - }, - "panels": [] - }, - { - "id": 8, + "id": 2, "type": "stat", - "title": "Added", + "title": "Merged", "datasource": { "type": "loki", "uid": "${loki}" @@ -377,8 +151,8 @@ "gridPos": { "h": 5, "w": 4, - "x": 0, - "y": 32 + "x": 4, + "y": 0 }, "fieldConfig": { "defaults": { @@ -417,15 +191,15 @@ "uid": "${loki}" }, "editorMode": "code", - "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap added [$__range]))", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap merged [$__range]))", "queryType": "instant" } ] }, { - "id": 9, + "id": 3, "type": "stat", - "title": "Merged", + "title": "Replaced", "datasource": { "type": "loki", "uid": "${loki}" @@ -433,8 +207,8 @@ "gridPos": { "h": 5, "w": 4, - "x": 4, - "y": 32 + "x": 8, + "y": 0 }, "fieldConfig": { "defaults": { @@ -473,15 +247,15 @@ "uid": "${loki}" }, "editorMode": "code", - "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap merged [$__range]))", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap replaced [$__range]))", "queryType": "instant" } ] }, { - "id": 10, + "id": 4, "type": "stat", - "title": "Replaced", + "title": "Skipped", "datasource": { "type": "loki", "uid": "${loki}" @@ -489,8 +263,8 @@ "gridPos": { "h": 5, "w": 4, - "x": 8, - "y": 32 + "x": 12, + "y": 0 }, "fieldConfig": { "defaults": { @@ -529,15 +303,15 @@ "uid": "${loki}" }, "editorMode": "code", - "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap replaced [$__range]))", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap skipped [$__range]))", "queryType": "instant" } ] }, { - "id": 11, + "id": 5, "type": "stat", - "title": "Skipped", + "title": "Total tiles", "datasource": { "type": "loki", "uid": "${loki}" @@ -545,8 +319,8 @@ "gridPos": { "h": 5, "w": 4, - "x": 12, - "y": 32 + "x": 16, + "y": 0 }, "fieldConfig": { "defaults": { @@ -585,69 +359,13 @@ "uid": "${loki}" }, "editorMode": "code", - "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap skipped [$__range]))", + "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap total [$__range]))", "queryType": "instant" } ] }, { - "id": 12, - "type": "stat", - "title": "Total tiles", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "gridPos": { - "h": 5, - "w": 4, - "x": 16, - "y": 32 - }, - "fieldConfig": { - "defaults": { - "unit": "short", - "color": { - "mode": "thresholds" - }, - "thresholds": { - "mode": "absolute", - "steps": [ - { - "color": "blue", - "value": null - } - ] - } - }, - "overrides": [] - }, - "options": { - "reduceOptions": { - "calcs": [ - "lastNotNull" - ], - "fields": "", - "values": false - }, - "colorMode": "value", - "graphMode": "area" - }, - "targets": [ - { - "refId": "A", - "datasource": { - "type": "loki", - "uid": "${loki}" - }, - "editorMode": "code", - "expr": "sum(last_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | message=~\".*Merge report.*\" | unwrap total [$__range]))", - "queryType": "instant" - } - ] - }, - { - "id": 13, + "id": 6, "type": "stat", "title": "p95 duration", "datasource": { @@ -658,7 +376,7 @@ "h": 5, "w": 4, "x": 20, - "y": 32 + "y": 0 }, "fieldConfig": { "defaults": { @@ -711,7 +429,7 @@ ] }, { - "id": 14, + "id": 7, "type": "timeseries", "title": "Tiles by outcome (per $__interval)", "datasource": { @@ -722,7 +440,7 @@ "h": 8, "w": 12, "x": 0, - "y": 37 + "y": 5 }, "fieldConfig": { "defaults": { @@ -791,7 +509,7 @@ ] }, { - "id": 15, + "id": 8, "type": "timeseries", "title": "Task duration (s)", "datasource": { @@ -802,7 +520,7 @@ "h": 8, "w": 12, "x": 12, - "y": 37 + "y": 5 }, "fieldConfig": { "defaults": { @@ -847,9 +565,9 @@ ] }, { - "id": 16, + "id": 9, "type": "table", - "title": "Per-task merge reports", + "title": "Per-task merge reports (click job/task to filter)", "datasource": { "type": "loki", "uid": "${loki}" @@ -858,7 +576,7 @@ "h": 10, "w": 24, "x": 0, - "y": 45 + "y": 13 }, "fieldConfig": { "defaults": { @@ -867,7 +585,44 @@ "align": "auto" } }, - "overrides": [] + "overrides": [ + { + "matcher": { + "id": "byName", + "options": "job" + }, + "properties": [ + { + "id": "links", + "value": [ + { + "title": "Filter dashboard to this job/task", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "targetBlank": false + } + ] + } + ] + }, + { + "matcher": { + "id": "byName", + "options": "task" + }, + "properties": [ + { + "id": "links", + "value": [ + { + "title": "Filter dashboard to this job/task", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "targetBlank": false + } + ] + } + ] + } + ] }, "options": { "showHeader": true, @@ -929,6 +684,262 @@ } } ] + }, + { + "id": 10, + "type": "logs", + "title": "Task logs", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 11, + "w": 16, + "x": 0, + "y": 23 + }, + "options": { + "showTime": true, + "showLabels": false, + "showCommonLabels": false, + "wrapLogMessage": true, + "prettifyLogMessage": false, + "enableLogDetails": true, + "dedupStrategy": "none", + "sortOrder": "Descending" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\"", + "queryType": "range" + } + ] + }, + { + "id": 11, + "type": "timeseries", + "title": "Log rate by level", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 6, + "w": 8, + "x": 16, + "y": 23 + }, + "fieldConfig": { + "defaults": { + "custom": { + "drawStyle": "bars", + "fillOpacity": 40, + "stacking": { + "mode": "normal" + } + } + }, + "overrides": [] + }, + "options": { + "legend": { + "displayMode": "list", + "placement": "bottom", + "showLegend": true + } + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum by (level) (count_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" [$__interval]))", + "queryType": "range", + "legendFormat": "{{level}}" + } + ] + }, + { + "id": 12, + "type": "stat", + "title": "Log lines (range)", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 5, + "w": 8, + "x": 16, + "y": 29 + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "green", + "value": null + } + ] + } + }, + "overrides": [] + }, + "options": { + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "value", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(count_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 13, + "type": "logs", + "title": "Errors & warnings", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 8, + "w": 24, + "x": 0, + "y": 34 + }, + "options": { + "showTime": true, + "showLabels": false, + "showCommonLabels": false, + "wrapLogMessage": true, + "prettifyLogMessage": false, + "enableLogDetails": true, + "dedupStrategy": "none", + "sortOrder": "Descending" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | level=~\"Error|Warning|Critical\"", + "queryType": "range" + } + ] + }, + { + "id": 14, + "type": "table", + "title": "Log timeline (parsed)", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 10, + "w": 24, + "x": 0, + "y": 42 + }, + "fieldConfig": { + "defaults": { + "custom": { + "filterable": true, + "align": "auto" + } + }, + "overrides": [] + }, + "options": { + "showHeader": true, + "cellHeight": "sm", + "footer": { + "show": false + } + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\"", + "queryType": "range" + } + ], + "transformations": [ + { + "id": "organize", + "options": { + "excludeByName": { + "service": true, + "version": true, + "thread": true, + "service_name": true, + "k8s_container_name": true, + "k8s_deployment_name": true, + "k8s_namespace_name": true, + "k8s_pod_name": true, + "time": true, + "Line": true, + "id": true, + "tsNs": true, + "labels": true, + "jobId": true, + "taskId": true, + "added": true, + "merged": true, + "replaced": true, + "skipped": true, + "total": true, + "durationSeconds": true + }, + "indexByName": { + "Time": 0, + "level": 1, + "category": 2, + "message": 3 + }, + "renameByName": {} + } + } + ] } ], "annotations": { From 260faad88fa2c5620804bfc2c5eafad770bac261 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 20:46:40 +0300 Subject: [PATCH 22/27] fix(dashboard): extract json fields into table columns; add error count + messages-only panel Table panels showed only Time because Loki | json fields land inside the labels map, not as dataframe columns. Add an extractFields transform ahead of organize so job/task/added/merged/... surface as columns. Add an Errors (range) stat and an Error messages table (Time/job/task/error only) so failures are visible at a glance. Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 174 +++++++++++++++++++-- 1 file changed, 159 insertions(+), 15 deletions(-) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index 013273b..2ea34d6 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -644,6 +644,14 @@ } ], "transformations": [ + { + "id": "extractFields", + "options": { + "source": "labels", + "replace": false, + "keepTime": false + } + }, { "id": "organize", "options": { @@ -658,9 +666,9 @@ "k8s_pod_name": true, "time": true, "Line": true, + "labels": true, "id": true, "tsNs": true, - "labels": true, "level": true, "category": true, "message": true @@ -825,28 +833,107 @@ ] }, { - "id": 13, - "type": "logs", - "title": "Errors & warnings", + "id": 15, + "type": "stat", + "title": "Errors (range)", "datasource": { "type": "loki", "uid": "${loki}" }, "gridPos": { "h": 8, - "w": 24, + "w": 4, "x": 0, "y": 34 }, + "fieldConfig": { + "defaults": { + "unit": "short", + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "green", + "value": null + }, + { + "color": "red", + "value": 1 + } + ] + } + }, + "overrides": [] + }, "options": { - "showTime": true, - "showLabels": false, - "showCommonLabels": false, - "wrapLogMessage": true, - "prettifyLogMessage": false, - "enableLogDetails": true, - "dedupStrategy": "none", - "sortOrder": "Descending" + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, + "colorMode": "background", + "graphMode": "area" + }, + "targets": [ + { + "refId": "A", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "editorMode": "code", + "expr": "sum(count_over_time({k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | level=~\"Error|Critical\" [$__range]))", + "queryType": "instant" + } + ] + }, + { + "id": 13, + "type": "table", + "title": "Error messages", + "datasource": { + "type": "loki", + "uid": "${loki}" + }, + "gridPos": { + "h": 8, + "w": 20, + "x": 4, + "y": 34 + }, + "fieldConfig": { + "defaults": { + "custom": { + "filterable": true, + "align": "auto" + } + }, + "overrides": [ + { + "matcher": { + "id": "byName", + "options": "message" + }, + "properties": [ + { + "id": "custom.width", + "value": 900 + } + ] + } + ] + }, + "options": { + "showHeader": true, + "cellHeight": "sm", + "footer": { + "show": false + } }, "targets": [ { @@ -856,9 +943,58 @@ "uid": "${loki}" }, "editorMode": "code", - "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | level=~\"Error|Warning|Critical\"", + "expr": "{k8s_namespace_name=~\"$namespace\", k8s_deployment_name=~\"gpkg-merger.*\"} | json | __error__=\"\" | jobId=~\"$job\" | taskId=~\"$task\" | level=~\"Error|Critical\"", "queryType": "range" } + ], + "transformations": [ + { + "id": "extractFields", + "options": { + "source": "labels", + "replace": false, + "keepTime": false + } + }, + { + "id": "organize", + "options": { + "excludeByName": { + "service": true, + "version": true, + "thread": true, + "service_name": true, + "k8s_container_name": true, + "k8s_deployment_name": true, + "k8s_namespace_name": true, + "k8s_pod_name": true, + "time": true, + "Line": true, + "labels": true, + "id": true, + "tsNs": true, + "category": true, + "level": true, + "added": true, + "merged": true, + "replaced": true, + "skipped": true, + "total": true, + "durationSeconds": true + }, + "indexByName": { + "Time": 0, + "jobId": 1, + "taskId": 2, + "message": 3 + }, + "renameByName": { + "jobId": "job", + "taskId": "task", + "message": "error" + } + } + } ] }, { @@ -904,6 +1040,14 @@ } ], "transformations": [ + { + "id": "extractFields", + "options": { + "source": "labels", + "replace": false, + "keepTime": false + } + }, { "id": "organize", "options": { @@ -918,9 +1062,9 @@ "k8s_pod_name": true, "time": true, "Line": true, + "labels": true, "id": true, "tsNs": true, - "labels": true, "jobId": true, "taskId": true, "added": true, From ea585247594b75c4c241b41b435f350a5c522c65 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 20:51:57 +0300 Subject: [PATCH 23/27] fix(dashboard): move error panels to second row above logs Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index 2ea34d6..53202cf 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -440,7 +440,7 @@ "h": 8, "w": 12, "x": 0, - "y": 5 + "y": 13 }, "fieldConfig": { "defaults": { @@ -520,7 +520,7 @@ "h": 8, "w": 12, "x": 12, - "y": 5 + "y": 13 }, "fieldConfig": { "defaults": { @@ -576,7 +576,7 @@ "h": 10, "w": 24, "x": 0, - "y": 13 + "y": 21 }, "fieldConfig": { "defaults": { @@ -705,7 +705,7 @@ "h": 11, "w": 16, "x": 0, - "y": 23 + "y": 31 }, "options": { "showTime": true, @@ -742,7 +742,7 @@ "h": 6, "w": 8, "x": 16, - "y": 23 + "y": 31 }, "fieldConfig": { "defaults": { @@ -789,7 +789,7 @@ "h": 5, "w": 8, "x": 16, - "y": 29 + "y": 37 }, "fieldConfig": { "defaults": { @@ -844,7 +844,7 @@ "h": 8, "w": 4, "x": 0, - "y": 34 + "y": 5 }, "fieldConfig": { "defaults": { @@ -904,7 +904,7 @@ "h": 8, "w": 20, "x": 4, - "y": 34 + "y": 5 }, "fieldConfig": { "defaults": { From f1037a1ca2bb9c73aa1e8d5848649e69ddc04b44 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 20:56:56 +0300 Subject: [PATCH 24/27] fix(dashboard): per-task table to third row; clickable job/task in error table Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 44 ++++++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index 53202cf..3eecc26 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -440,7 +440,7 @@ "h": 8, "w": 12, "x": 0, - "y": 13 + "y": 23 }, "fieldConfig": { "defaults": { @@ -520,7 +520,7 @@ "h": 8, "w": 12, "x": 12, - "y": 13 + "y": 23 }, "fieldConfig": { "defaults": { @@ -576,7 +576,7 @@ "h": 10, "w": 24, "x": 0, - "y": 21 + "y": 13 }, "fieldConfig": { "defaults": { @@ -917,7 +917,7 @@ { "matcher": { "id": "byName", - "options": "message" + "options": "error" }, "properties": [ { @@ -925,6 +925,42 @@ "value": 900 } ] + }, + { + "matcher": { + "id": "byName", + "options": "job" + }, + "properties": [ + { + "id": "links", + "value": [ + { + "title": "Filter dashboard to this job/task", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "targetBlank": false + } + ] + } + ] + }, + { + "matcher": { + "id": "byName", + "options": "task" + }, + "properties": [ + { + "id": "links", + "value": [ + { + "title": "Filter dashboard to this job/task", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "targetBlank": false + } + ] + } + ] } ] }, From cef64601a020877810a93d257d80c42d502fc453 Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 21:13:35 +0300 Subject: [PATCH 25/27] fix(dashboard): show 'No data' on report tiles for tasks with no merge report Failed tasks emit no merge-report line, so report-based panels are empty when drilled into. Make the stat tiles say 'No data' instead of rendering blank. Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index 3eecc26..54bab17 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -101,6 +101,7 @@ "fieldConfig": { "defaults": { "unit": "short", + "noValue": "No data", "color": { "mode": "thresholds" }, @@ -157,6 +158,7 @@ "fieldConfig": { "defaults": { "unit": "short", + "noValue": "No data", "color": { "mode": "thresholds" }, @@ -213,6 +215,7 @@ "fieldConfig": { "defaults": { "unit": "short", + "noValue": "No data", "color": { "mode": "thresholds" }, @@ -269,6 +272,7 @@ "fieldConfig": { "defaults": { "unit": "short", + "noValue": "No data", "color": { "mode": "thresholds" }, @@ -325,6 +329,7 @@ "fieldConfig": { "defaults": { "unit": "short", + "noValue": "No data", "color": { "mode": "thresholds" }, @@ -381,6 +386,7 @@ "fieldConfig": { "defaults": { "unit": "s", + "noValue": "No data", "color": { "mode": "thresholds" }, From 309a26c5680b0795eab23986bc400a992c8b76ec Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 21:16:51 +0300 Subject: [PATCH 26/27] fix(dashboard): job cell fills job only (clears task); task cell fills both Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index 54bab17..6251afc 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -602,8 +602,8 @@ "id": "links", "value": [ { - "title": "Filter dashboard to this job/task", - "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "title": "Filter dashboard to this job", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=", "targetBlank": false } ] @@ -620,7 +620,7 @@ "id": "links", "value": [ { - "title": "Filter dashboard to this job/task", + "title": "Filter dashboard to this job and task", "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", "targetBlank": false } @@ -942,8 +942,8 @@ "id": "links", "value": [ { - "title": "Filter dashboard to this job/task", - "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "title": "Filter dashboard to this job", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=", "targetBlank": false } ] @@ -960,7 +960,7 @@ "id": "links", "value": [ { - "title": "Filter dashboard to this job/task", + "title": "Filter dashboard to this job and task", "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", "targetBlank": false } From 489e5213cef035c5ff750761439d38f4e59fb22a Mon Sep 17 00:00:00 2001 From: shimoncohen Date: Thu, 17 Sep 2026 21:27:54 +0300 Subject: [PATCH 27/27] fix(dashboard): carry loki datasource + namespace through drill-down links Data links did not pass var-loki, and the datasource variable's saved current is empty, so navigating via a link left $loki unresolved -> 'Failed to upgrade legacy queries'. Carry var-loki and the namespace selection in every link. Co-Authored-By: Claude Opus 4.8 (1M context) --- Assets/Dashboards/MergerLogsDashboard.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Assets/Dashboards/MergerLogsDashboard.json b/Assets/Dashboards/MergerLogsDashboard.json index 6251afc..c0dd0df 100644 --- a/Assets/Dashboards/MergerLogsDashboard.json +++ b/Assets/Dashboards/MergerLogsDashboard.json @@ -1,5 +1,5 @@ { - "title": "GPKG-Merger \u2014 Logs", + "title": "GPKG-Merger — Logs", "uid": "gpkg-merger-logs", "tags": [ "gpkg-merger", @@ -603,7 +603,7 @@ "value": [ { "title": "Filter dashboard to this job", - "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-loki=${loki}&${namespace:queryparam}&var-job=${__data.fields.job}&var-task=", "targetBlank": false } ] @@ -621,7 +621,7 @@ "value": [ { "title": "Filter dashboard to this job and task", - "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-loki=${loki}&${namespace:queryparam}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", "targetBlank": false } ] @@ -943,7 +943,7 @@ "value": [ { "title": "Filter dashboard to this job", - "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-loki=${loki}&${namespace:queryparam}&var-job=${__data.fields.job}&var-task=", "targetBlank": false } ] @@ -961,7 +961,7 @@ "value": [ { "title": "Filter dashboard to this job and task", - "url": "/d/gpkg-merger-logs?${__url_time_range}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", + "url": "/d/gpkg-merger-logs?${__url_time_range}&var-loki=${loki}&${namespace:queryparam}&var-job=${__data.fields.job}&var-task=${__data.fields.task}", "targetBlank": false } ]