Skip to content

fix: tracking plan merged config is cached without its event type - #7314

Open
atzoum wants to merge 1 commit into
masterfrom
fix.trackingPlanMergedConfig
Open

fix: tracking plan merged config is cached without its event type#7314
atzoum wants to merge 1 commit into
masterfrom
fix.trackingPlanMergedConfig

Conversation

@atzoum

@atzoum atzoum commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Description

DgSourceTrackingPlanConfigT.GetMergedConfig(eventType) layers the tracking plan's config for an event type over its global config, and memoised the result into the source's own DgSourceTrackingPlanConfig.MergedConfig.

Caching it there is both wrong and dangerous. Wrong, because the field has no room for the event type it was merged for, so the first caller's answer is handed to every later one:

track    -> map[allowUnplannedEvents:true propertiesRule:track-rules]
identify -> map[allowUnplannedEvents:true propertiesRule:track-rules]   <- identify's rules never appear
page     -> map[allowUnplannedEvents:true propertiesRule:track-rules]

Dangerous, because computing a merge should not write into the source config at all - and there is no need for it to: the merge is two map lookups and an lo.Assign, recomputed per event today regardless, since the memo is written once per source copy and thrown away.

It does not misfire as things stand (one event per gateway batch, a fresh source copy per job), but mergedTpConfig is what decides whether an event is dropped - rudder-transformer dispatches allowUnplannedEvents, unplannedProperties, anyOtherViolation and sendViolatedEventsTo off it, per event - so the cost of it ever misfiring is event loss rather than noise.

MergedConfig is removed with it: never on the wire in either config version, and nothing outside the getter read it. fetchEventConfig goes too - it turned an absent key into an empty map, which lo.Assign does by itself.

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@atzoum
atzoum force-pushed the fix.trackingPlanMergedConfig branch from fd353cc to 38a8564 Compare August 27, 2026 13:10
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.37%. Comparing base (267b973) to head (6c3b616).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7314      +/-   ##
==========================================
+ Coverage   79.34%   79.37%   +0.03%     
==========================================
  Files         609      609              
  Lines       67963    67973      +10     
==========================================
+ Hits        53922    53953      +31     
+ Misses      10876    10841      -35     
- Partials     3165     3179      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@atzoum
atzoum force-pushed the fix.trackingPlanMergedConfig branch from 38a8564 to 7388732 Compare August 27, 2026 13:52
GetMergedConfig merges the tracking plan's global config with the config of the event type it is
asked about, and memoised the result in DgSourceTrackingPlanConfigT.MergedConfig - a field with no
room for the event type it was merged for. The first caller's answer was therefore returned to
every later one, whatever event type it asked about.

It does not misfire today: a gateway batch holds a single event and the processor takes a fresh
copy of the source per job, so the memo is written once and thrown away. It would misfire the
moment one source value serves two event types, and mergedTpConfig decides whether an event is
dropped, so the cost of that would be event loss rather than noise.

Drop the memo and the field with it: the merge is two map lookups and an lo.Assign, and it was
already being recomputed for every event.
@atzoum
atzoum force-pushed the fix.trackingPlanMergedConfig branch from 7388732 to 6c3b616 Compare August 28, 2026 07:12

@mihir20 mihir20 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants