Add SEQ properties, fix cache loop and other improvements - #16
Conversation
|
Hi! Thanks for opening a PR! I will need some time to review everything. Meanwhile, you can make sure you reviewed the CONTRIBUTING.md (particularly formatting and writing tests). |
ricardoboss
left a comment
There was a problem hiding this comment.
Again, thanks for opening a PR!
This applies here as well: I'm not a big fan of such big PRs, but I am happy you put in the effort.
I have a few suggestions and questions. Also, please review the comment on seq_logger_test.dart.
|
It seems you need to run |
|
@petrnymsa you need to rebase, I fixed a small issue on |
Per-event success/failure tracking with isPermanent flag to distinguish malformed events from transient errors.
Change SeqClient.sendEvents() return type from Future<void> to Future<List<SeqEventSentResult>>. Throws on total failure, returns mixed results on partial failure.
Rewrite flush() with two paths: partial failure (per-event results) and total failure (exception). Add throwOnError, flushInterval, dispose(). Change FlushErrorHandler typedef to receive List<SeqEventSentResult>. Default behavior drops permanent failures and re-queues transient ones. # Conflicts: # lib/src/seq_logger.dart
Update mock clients to return List<SeqEventSentResult>. Add tests for: partial failure with isPermanent, transient re-queue, total failure cache retention, onFlushError with synthetic results, and isPermanent flag on SeqEventSentResult.
Document flush paths, isPermanent flag, flush triggers, configuration flags, and recommended onFlushError usage. Update CHANGELOG with all v3.0.0 breaking changes and features.
|
@ricardoboss Since we doing breaking change, are you ok with bumping Dart SDK version? We could use null-aware-elements to simplify code |
Yeah, that's ok. |
682bf03 to
d049185
Compare
|
@ricardoboss I've tried to simplify code, removed non-sense tests. Also dropped flushTimer "feature" |
ricardoboss
left a comment
There was a problem hiding this comment.
We're almost there. Thanks for your continued efforts on this!
ricardoboss
left a comment
There was a problem hiding this comment.
Very nice! Thanks for making the changes
|
@petrnymsa both |
|
Thank you for quick review |
First of all, thank you for this package.
We used dart_seq heavily for last year in our big projects (over 1M+ Month users) and found several issues.
Main issue is loop-hole where event can be potentially malformed which is refused by SEQ server (Bad Request 400). There was no way to tell that this happened and next time, same event was read from cache - thus creating infinite loophole of failure. From this point, no more logs were written to SEQ.
Originally I wanted to create multiple several, focused PRs but I've decided (And yes with help of AI agents) to tackle multiple issues at once.
I am more than happy to fine-tune it before we merge it, but we will definitely use "new" version.
Note that there are is PR for dart_seq_http_client too. I will update version constraints after we agree that this PR makes sense.
Motivation
The original
dart_seqhad several pain points when used in production:Logging methods could crash the app (#13, #14) —
SeqClient.sendEvents()returnedFuture<void>with no error isolation. A network error or server rejection during flush would throw an unhandled exception, potentially crashing the host app. Users had no way to choose between "fire and forget" and "audit" logging.Oversized payloads caused infinite retry loops (#12) — when the Seq server rejected a batch (e.g. HTTP 413 Payload Too Large), the events stayed in cache and were re-sent on every flush. This blocked all subsequent logging indefinitely, because the same oversized batch was retried forever.
No support for OpenTelemetry / distributed tracing fields (#10) — Seq supports
@tr,@sp,@ps,@st,@sc,@ra,@skCLEF fields for distributed tracing, but the library had no way to set them. Passing them via context caused double-escaping (@trbecame@@tr), making them unusable.No timer-based auto-flush (#15) — events that didn't reach the
backlogLimitthreshold would sit in cache indefinitely until the next batch filled up. There was no way to ensure logs were sent after a period of inactivity.Positional parameters made the API hard to extend (#10) — adding new fields to
log()required breaking positional parameter order. Named parameters are needed for a maintainable API surface.This PR addresses all of the above.
Closes Issues
@tr,@sp,@ps,@st,@sc,@ra,@sk)Error.safeToString→toString())throwOnErrorflag)flushInterval)Breaking Changes
SeqLogger.log()—exceptionandcontextchanged from positional to named parametersSeqLoggerconvenience methods (verbose,debug,info,warning,error,fatal) — same positional-to-named migrationSeqEventconstructor — all parameters excepttimestampare now namedSeqClient.sendEvents()— return type changed fromFuture<void>toFuture<List<SeqEventSentResult>>(implementations must return per-event results)Changes
Per-event error isolation
Why: Previously,
flush()treated all events as a single unit — either everything succeeded or everything failed. A single malformed event in a batch would cause the entire batch to fail and be retried forever, blocking all logging.What:
SeqEventSentResultclass — carriesisSuccess,error, andisPermanentper eventflush()Path A now processes per-event results: permanent failures (e.g. malformed events rejected with HTTP 400) are dropped, transient failures are re-queuedNon-retryable error handling (Path B)
Why: When
sendEventsthrows (total failure), all events stayed in cache unconditionally. For non-retryable errors like 413 (Payload Too Large), this meant the same oversized batch was re-sent on every flush, blocking the entire logging pipeline indefinitely (#12).What:
SeqClientException.isRetryablegetter (default:true) — allows subclasses to signal non-retryable errorssendEventsthrows a non-retryableSeqClientException:_nextFlushBatchSizehalves on each failure, converging to single-event testingbacklogLimitafter a successful flushExceptions retain existing behavior (events stay in cache)onFlushErrorcallbackWhy: The built-in defaults (drop permanent, re-queue transient) cover the common cases, but some apps need custom behavior — e.g. logging individual failures, applying retry limits, or reporting to crash analytics (#13).
What:
FlushErrorHandleronSeqLogger— receives per-event results and error, returns events to re-queuenull, built-in defaults applythrowOnErrorflag (#14)Why: The original library had no way to choose between "fire and forget" and "audit" logging. Some apps want logging to never crash the app; others want to know immediately when logging fails.
What:
false(default): flush errors are caught and reported viaonDiagnosticLog— safe for productiontrue: exceptions propagate to caller — useful for debugging or when logging failures must be handledflushIntervaltimer (#15)Why: Events that didn't reach the
backlogLimitthreshold sat in cache indefinitely. In low-traffic apps, important logs (like a single error) could wait minutes or hours before being flushed.What:
Durationthat triggers automatic flush after inactivitysend()calldispose()method cancels the timerException formatting fix (#11)
Why: The
@x(exception) CLEF field was serialized usingError.safeToString(), which wraps non-String objects inInstance of 'Foo'and escapes newlines as literal\ntext. This made stack traces and custom exception messages unreadable in Seq.What:
Error.safeToString(exception)with a_safeToString()helper that triestoString()first and falls back toError.safeToString()if it throwstoString()overrides now render properly in Seq\ntexttoString()implementations — no risk of crashing the serializerOpenTelemetry / distributed tracing fields (#10)
Why: Seq supports W3C trace context fields for distributed tracing, but the library had no native support. Passing them via context caused double-escaping (
@tr→@@tr), making Seq's tracing tools unusable.What:
SeqEvent:traceId(@tr),spanId(@sp),parentSpanId(@ps),spanStart(@st),scope(@sc),resourceAttributes(@ra),spanKind(@sk)SeqLogger.log()for all tracing fieldsFiles Changed
lib/dart_seq.dartSeqEventSentResultlib/src/seq_client.dartFuture<List<SeqEventSentResult>>, expanded DartDoclib/src/seq_client_exception.dartisRetryablegetterlib/src/seq_event.dartfromMap(),withAddedContext()lib/src/seq_event_sent_result.dartlib/src/seq_logger.dartonFlushError,throwOnError,flushInterval,dispose(), Path A/B rewrite,_nextFlushBatchSize, diagnostic logging for dropped eventspubspec.yamlfake_asyncdev dependencyCHANGELOG.mdREADME.mdonFlushErrorexampleTest Coverage
main)seq_event_sent_result_test.dart,seq_client_exception_test.dartseq_logger_test.dart(+1295 lines),seq_event_test.dart(+681 lines),seq_in_memory_cache_test.dart(+165 lines)onFlushErrorcallback integration (total and partial failure)