fix: do not log an error per evaluation for an absent string comparison property - #2019
Draft
scottt732 wants to merge 1 commit into
Draft
fix: do not log an error per evaluation for an absent string comparison property#2019scottt732 wants to merge 1 commit into
scottt732 wants to merge 1 commit into
Conversation
…on property Fixes open-feature#2018. A `starts_with` / `ends_with` rule referencing a context attribute the client did not send logged at error level, with a stack trace, on every evaluation. jsonLogic resolves a missing `var` to nil, and the parse helper could not tell that apart from a wrong-typed operand, so both took the same error path. Two properties of that path made it expensive. It runs per evaluation, so its volume tracks request rate rather than the number of bad rules; and zap.Config.Build attaches a stack trace at error level, turning each occurrence into roughly forty log lines. A rule that ORs several version prefixes multiplies again -- one of ours had eight starts_with calls, so a single bulk OFREP evaluation produced several hundred lines. That combination exhausted our organisation's daily log-index quota in about nine minutes, stopping log indexing for every service we run until the quota reset. The flag itself resolved correctly throughout; the entire cost was log volume. parseStringComparisonEvaluationData now returns a distinct sentinel when the property operand is nil, letting the two cases be reported differently: an absent attribute at debug, since referencing an optional attribute is ordinary and the rule simply does not match, and a malformed rule at warn, which still surfaces a real misconfiguration but without the stack trace. Both continue to return nil to jsonLogic, so evaluation results are unchanged. The two evaluators now share the comparison and logging path, which keeps that policy in one place. Verified against the reproduction in open-feature#2018: evaluation output is byte-identical for absent, matching and wrong-typed operands, while the absent case now emits no log line at default level and the wrong-typed case emits one warn with no stack trace -- previously an error plus ~40 stack frames each. Signed-off-by: Scott Holodak <scottt732@gmail.com>
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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.
Fixes #2018. Opening as a draft since #2018 proposes a direction and I'd rather confirm it before you spend review time — happy to change the severities, the sentinel, or the shared-helper refactor.
The problem
A
starts_with/ends_withrule referencing a context attribute the client did not send logs at error level, with a stack trace, on every evaluation. jsonLogic resolves a missingvartonil, andparseStringComparisonEvaluationDatacould not tell that apart from a wrong-typed operand, so both took the same error path.Two properties of that path make it expensive:
zap.Config.Build()attaches a stack trace at error level, turning each occurrence into ~40 log linesA rule that ORs several version prefixes multiplies again — one of ours had eight
starts_withcalls, so a single bulk OFREP evaluation produced several hundred lines. That exhausted our organisation's daily log-index quota in about nine minutes and stopped log indexing for every service we run until the quota reset. The flag resolved correctly throughout; the entire cost was log volume.The change
parseStringComparisonEvaluationDatareturns a distinct sentinel when the property operand isnil, which lets the two cases be reported differently:error+ stack trace, per evaluationdebugerror+ stack trace, per evaluationwarn, no stack traceRationale: referencing an optional attribute is ordinary and the rule simply does not match, so it isn't an error condition at all. A wrong-typed operand is a real misconfiguration and stays visible — but at a level that doesn't attach a stack trace, since the volume is still driven by request rate.
Both continue to return
nilto jsonLogic, so evaluation results are unchanged. The two evaluators now share the comparison and logging path, keeping that policy in one place.Testing
New cases cover the absent/wrong-type distinction, that a wrong-typed operand is not treated as absent, that neither errors the evaluation, and that ordinary string comparison still works. Full
coreandflagdsuites pass.Verified against the reproduction in #2018 — evaluation output byte-identical, log output transformed:
Open questions
warnto keep it visible without the stack trace. If you'd rather keeperrorthere, a log-once/deduplicated variant would be needed to stop the volume tracking request rate — happy to add that instead.TARGETING_MATCHreason. As noted in [BUG] starts_with/ends_with logs an ERROR with a stack trace on every evaluation when the property is absent #2018, all three cases above reportTARGETING_MATCHwith noerrorCode, so an operator error is indistinguishable from a legitimate else-branch match to SDKs and to validation tooling. That's a behaviour change rather than a logging fix, so I've deliberately left it out of this PR and am happy to follow up separately if you want to pursue it.