Conversation
The SUMMARY branch in ReportPipeline.process() was writing dedup summaries to the report file but not forwarding them to the external logger (stacktale.reports), unlike the BLOCK branch. This caused downstream aggregators to miss recurrence events. - Extract rendered summary into a local variable - Call host.emitReport(rendered) after confirmWritten, mirroring BLOCK - Add integration test: two identical errors -> assert second logger event contains 'repeated 2x' and is not a full block - Update README emitReportsToLogger description
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
GabrielBBaldez
left a comment
GabrielBBaldez
left a comment
There was a problem hiding this comment.
The placement is the part that was easy to get wrong, and you got it right without being told: after writer.append and after confirmWritten, so a shipper that throws cannot undo dedup state. That is the contract the REPORT arm spells out two lines above, and this arm now honours it.
I ran it rather than reading it. Your test passes, and removing just the emission line makes it fail for the right reason:
Expected size: 2 but was: 1
One thing to settle first, and it is the README line rather than the code.
renderSummary is called from two places. The one you changed runs while events are flowing; the other is in close(), draining whatever the dedup window still had pending when the app shut down. Five identical errors, emitReportsToLogger=true:
file: ━ #4359adb0 repeated 2× (last 16:44:34.646) ━
━ #4359adb0 repeated 5× (last 16:44:34.650) ━
logger: ━━━ ERROR #4359adb0 ━━━ …
━ #4359adb0 repeated 2× (last 16:44:34.646) ━
The final count — the one that says this error happened five times, not twice — is the one that stays behind. So Also emit each report block and recurrence summary as ONE event promises more than the code delivers, which is the shape of the problem #234 was about to begin with.
Either way out is fine by me:
- narrow the README sentence to the summaries emitted during the run, or
- emit at the drain site as well. I measured that rather than assuming it — adding the call there took the probe from 2 events to 3, so a Logback context on its way down is not in the way. A JVM shutdown hook is a harder case and I have not measured that one.
The second is still one logical change — summaries reach the logger — so it belongs in this PR rather than a follow-up, if you want it.
Not yours, and mine to deal with: the storm: line has the same shape. Written to the file, never emitted, and it is the line that says reports are being dropped.
One small thing: the template's Closes # is still blank, so #234 will not close itself when this merges.
|
Followed through on the storm line rather than leaving it as a remark: #238, measured the same way. It touches the arm right above yours, so it waits on this one. |
|
Still open on the one question from the review, so to make it concrete — either of these closes it and I am happy with both:
Nothing else is outstanding. The pipeline change is right and your test demands it. No rush. If you have moved on, say so and I will take option 1 on top of your commits so it lands with your name on it. |
|
Heads up on a collision: #232 just landed and the new README Deployment section ends with
That is true on Still happy to take it from here if you have moved on — the offer stands. |
This branch has not been deployed
2 participants
The SUMMARY branch in ReportPipeline.process() was writing dedup summaries to the report file but not forwarding them to the external logger (stacktale.reports), unlike the BLOCK branch. This caused downstream aggregators to miss recurrence events.
What & why
Closes #
How it was verified
mvn verifyis green (JDK 17+)Checklist
CONTRIBUTING
— didn't claim it? Open the PR anyway, just say so)
failing test
st/1is a public API. If a golden test didchange, that is a deliberate format change: say so below, and update
docs/FORMAT.md
current behavior, coverage in
LogbackXmlConfigTest, and a row in the READMEconfig table
AI assistance (optional)