NMS-20204: Reduce per-event overhead in the event translator - #8779
Open
marshallmassengill wants to merge 3 commits into
Open
NMS-20204: Reduce per-event overhead in the event translator#8779marshallmassengill wants to merge 3 commits into
marshallmassengill wants to merge 3 commits into
Conversation
EventTranslatorConfigFactory repeated most of its work on every event. Value specs exposed matches() and getResult() as separate calls, and TranslationMapping.translate() invoked both, so each value was resolved twice per event. For a sql value that meant two connection checkouts, two prepared statements and two round trips, and nested values were evaluated three times. ValueSpec now exposes a single evaluate() that returns whether the value matched along with the value to assign. EvaluationResult carries the two separately rather than collapsing into an Optional, because a sql lookup that finds a row with a null column is a match whose value is null, which must not fall through to the assignment default. The matches regex was recompiled on every evaluation, and a "~"-prefixed parameter name was recompiled once per parm scanned via String.matches. Both are now compiled once when the value spec is constructed. translate() cloned the event before running any assignment, so a mapping that rejected the event still paid for the clone. Assignments are now resolved against the source event first, which is safe because value specs only ever read the source event, and the clone happens only once the mapping is known to match. The early exit on the first assignment that neither matches nor has a default is preserved, so a rejecting mapping does no more work than before. cloneEvent() deep-copied through a Java serialization round trip, which spent most of its time re-writing class descriptors: 52 us/op against 0.74 us/op for a field copy through the immutable event model. Event does not implement IEvent, so the copy goes via ImmutableMapper. Both mappers cover all 34 of Event's fields. m_translationSpecs is now volatile, since update() clears it while translateEvent() reads it without synchronization. No configuration, schema or interface changes.
marshallmassengill
requested review from
cgorantla,
christianpape,
dino2gnt and
indigo423
and
a lite review from Copilot
August 11, 2026 20:35
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces per-event overhead in EventTranslatorConfigFactory by avoiding duplicate work during translation (notably repeated SQL queries, repeated regex compilation, and unnecessary cloning), and adds focused regression tests for SQL translation semantics and event cloning fidelity.
Changes:
- Refactors translation evaluation to compute “match + value” once per
ValueSpecevaluation (avoids double SQL round trips) and delays cloning until after a mapping matches. - Compiles
matchespatterns and~parameter-name regexes once per spec construction rather than per event. - Replaces serialization-based
cloneEvent()with a deep copy via the immutable event model; adds new tests covering SQL-value behavior and clone fidelity.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| opennms-config/src/main/java/org/opennms/netmgt/config/EventTranslatorConfigFactory.java | Core translator refactor for single-pass evaluation, regex precompilation, delayed cloning, and new cloning implementation. |
| opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorSqlValueTest.java | Adds tests ensuring SQL value evaluation issues only one query per event and distinguishes “no row” vs “null column”. |
| opennms-config/src/test/java/org/opennms/netmgt/config/EventTranslatorCloneEventTest.java | Adds tests to ensure cloneEvent() preserves fields and is deep enough for independent mutation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
christianpape
requested changes
Aug 12, 2026
Read m_translationSpecs into a local before the null check. Returning the field directly hands back null when update() clears it in between, and translateEvent() iterates the result. Two threads racing a reload may now each construct a list, which is cheaper than putting a lock on the per-event path. Fix the misplaced quotes in the two getAttributeValue() debug messages.
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
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.
EventTranslatorConfigFactory repeated most of its work on every event.
Per trap on the shipped link-down config: 6 DB round trips → 3, regex compilations → 0.
Assisted by Anthropic Claude Opus 5.
External References