rudder-server icon indicating copy to clipboard operation
rudder-server copied to clipboard

fix(core): filter failed events in processor based on response codes

Open Sidddddarth opened this issue 3 years ago • 4 comments

Description

Filter failed events and simply drop in case of special response codes. Cases include GA - server side identify not on etc.

Notion Ticket

Filter identify if serverside identify is not on

Security

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

Sidddddarth avatar Aug 02 '22 09:08 Sidddddarth

Codecov Report

Merging #2230 (6535680) into master (08d9f8f) will increase coverage by 0.07%. The diff coverage is 70.24%.

:exclamation: Current head 6535680 differs from pull request most recent head 8605e7c. Consider uploading reports for the commit 8605e7c to get more accurate results

@@            Coverage Diff             @@
##           master    #2230      +/-   ##
==========================================
+ Coverage   33.25%   33.32%   +0.07%     
==========================================
  Files         192      192              
  Lines       38462    38546      +84     
==========================================
+ Hits        12791    12847      +56     
- Misses      24882    24912      +30     
+ Partials      789      787       -2     
Impacted Files Coverage Δ
processor/transformer/transformer.go 61.16% <ø> (ø)
processor/processor.go 69.83% <34.88%> (-0.82%) :arrow_down:
router/router.go 67.80% <89.74%> (+0.90%) :arrow_up:
enterprise/suppress-user/suppressUser.go 82.71% <0.00%> (-1.86%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Aug 11 '22 06:08 codecov[bot]

@Sidddddarth would you mind giving a bit more context about this PR? What is the motivation behind this change?

lvrach avatar Aug 11 '22 07:08 lvrach

@Sidddddarth would you mind giving a bit more context about this PR? What is the motivation behind this change?

https://rudderlabs.slack.com/archives/C0218DFG57D/p1657798156457899 https://rudderlabs.slack.com/archives/C02B0A38QQG/p1657721887565259 We're basically trying to add another layer of filtering to "bad" events. Similar to filtering events based on unsupported message types. The issue in question comes up because we filter out unsupported messages based on destination definition. But the actual destination configuration might differ from that, leading to some events pass along because they're supported(but the destination doesn't accept them). Example: GA - server side identify can be toggled while configuring the destination. But according to the GA destination definition, identify is a supported message type. In cases where these identify events reach the destination, we're alerted because of the aborts that this causes. This PR essentially tries to filter out such events. This is done based on the response code that the transformer returns.

Sidddddarth avatar Aug 11 '22 09:08 Sidddddarth

IMO suppressing events from router turns out to not being as straightforward as it is in processor. Processor is an event producer at heart, whereas router is an event consumer, so there is no easy way to suppress a message in router after it has been produced by processor. Moreover, processor already has an event filtering stage in place.

Can we reconsider our strategy & instead of relying on transformer for filtering out such events, either in processor or in router, do it always in processor during the event filtering stage?

E.g. the config plane could include some config option, following a uniform schema for defining event type whitelists/blacklists that we could use across destinations.

atzoum avatar Aug 11 '22 15:08 atzoum

We are moving forward with a different approach, as in #2211

atzoum avatar Aug 17 '22 08:08 atzoum

This PR is considered to be stale. It has been open 20 days with no further activity thus it is going to be closed in 5 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR.

github-actions[bot] avatar Sep 07 '22 02:09 github-actions[bot]