Reject maxBatchSize and flushPeriodMilliseconds below 1 and negative retries - #28
Merged
Merged
Conversation
A missing source token is caught by [RequiredParameter] on NLog 4 and 5 only, NLog 6 no longer checks it and sends the logs with an empty token. An empty endpoint is reported as a UriFormatException, which NLog 4 swallows even with throwConfigExceptions. A bare ingesting host, as shown in the source settings, is not reached at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[RequiredParameter] is obsolete in NLog 6 and no longer checked there. InitializeTarget now checks the rendered sourceToken and endpoint itself and throws NLogConfigurationException with a message that says what to set, the same on NLog 4.7, 5 and 6. An endpoint without an http:// or https:// scheme gets https:// in front, so the ingesting host works as the source settings show it. The example nlog.config and README spell out https:// all the same. Based on the configuration checks in #7. Co-authored-by: Rolf Kristensen <11509660+snakefoot@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An endpoint like "https://" has a scheme but no host, so new Uri() threw a UriFormatException in the Client, which NLog 4.7 only writes to the internal log and NLog 5 and 6 wrap in a generic initialization error. It is now reported as NLogConfigurationException that quotes the value as configured. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
maxBatchSize and flushPeriodMilliseconds of 0 and negative retries are accepted without a word. retries="0" stays valid. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…retries InitializeTarget reports them as NLogConfigurationException, after the sourceToken and endpoint checks. maxBatchSize="0" pinned a core without sending anything and flushPeriodMilliseconds="0" pinned one while idle. retries="0" stays valid. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Values of
maxBatchSize,flushPeriodMillisecondsandretriesthat make no sense are accepted without a word, and some of them cost a CPU core. Reproduced with a console app on .NET 10 and NLog 4.7.11 against a local receiver, measuring the process's CPU time over 3 seconds (the default configuration idles at about 1% of one core):maxBatchSize="0": idle while nothing is queued, then 93% of one core as soon as one log is written, and nothing is ever sent:Drain.flushloops for ever on a queue it never takes anything from.LogManager.Shutdown()then never returns, because stopping the drain waits for that loop.maxBatchSize="-1": no CPU cost, but every flush throwsArgumentOutOfRangeException(a list capacity of -1), which lands in the internal log each period, and nothing is ever sent.flushPeriodMilliseconds="0"(or negative): 100% of one core even with nothing logged, since the delay between flushes is zero and the loop never yields. Logs are delivered.retries="0"(or negative): no CPU cost, but no request is ever made and every batch is dropped.This PR checks the three values in
InitializeTarget, after thesourceTokenandendpointchecks of #22, and throwsNLogConfigurationExceptionwith a message naming the value, the same way:maxBatchSizeandflushPeriodMillisecondsbelow 1 are reported.retriesbelow 0 is reported.retries="0"stays valid: Retry as many times as retries says #23 makes it count the retries after the first attempt, so that 0 sends every batch once. This PR does not touch the retry loop, so on its ownretries="0"still sends nothing until Retry as many times as retries says #23 is merged too.The first commit adds the tests and is expected to be red: none of the three values is reported.
AcceptsZeroRetriespasses on both commits, it pins that 0 retries is not an error. The second commit adds the checks.ConfiguresFromXmlpasses unchanged.🤖 Generated with Claude Code