feat: support for config to custom destinations #2625

utsabc · 2022-10-27T11:01:56Z

Adding support for passing config to transformer for custom destinations

Description

Previously we were filtering the total config for requests to transformer for custom destinations. Main motivation for it was to avoid making heavy requests as some some configs would contain would contain certificate details etc.

This PR approaches the solution with more general intnet by filtering any destination config based on predefined filters set in integrations module

Notion Ticket

Notion link: https://www.notion.so/rudderstacks/Kafka-Destination-with-Multiple-Topics-85cbce26706d49eca6853b972e5497b0

Security

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

…assing config to transformer for custom destinations

utsabc · 2022-10-27T11:02:11Z

Related PR: #2569

codecov · 2022-10-27T11:17:55Z

Codecov Report

Base: 43.85% // Head: 43.84% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (b2ad586) compared to base (5c26d1b).
Patch coverage: 81.81% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2625      +/-   ##
==========================================
- Coverage   43.85%   43.84%   -0.01%     
==========================================
  Files         187      187              
  Lines       40027    40024       -3     
==========================================
- Hits        17552    17550       -2     
- Misses      21376    21378       +2     
+ Partials     1099     1096       -3

Impacted Files	Coverage Δ
utils/misc/misc.go	`12.54% <0.00%> (+0.01%)`	⬆️
processor/processor.go	`71.36% <100.00%> (+0.12%)`	⬆️
utils/httputil/server.go	`80.76% <0.00%> (-11.54%)`	⬇️
config/backend-config/namespace_config.go	`70.83% <0.00%> (-3.13%)`	⬇️
enterprise/reporting/reporting.go	`8.33% <0.00%> (-1.44%)`	⬇️
services/rsources/handler.go	`70.55% <0.00%> (+0.83%)`	⬆️
middleware/middleware.go	`94.82% <0.00%> (+8.62%)`	⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

koladilip

It looks good, but is this something that can be done from destination definitions itself? Why keep config at different places?

utsabc · 2022-10-27T13:40:02Z

It looks good, but is this something that can be done from destination definitions itself? Why keep config at different places?

Ideally yes for a quick fix I went with this appraoch

processor/processor.go

koladilip · 2022-10-28T04:40:22Z

It looks good, but is this something that can be done from destination definitions itself? Why keep config at different places?

Ideally yes for a quick fix I went with this appraoch

is this going to be tech debt again? I prefer to have this to be defined at destDef level so that it is easy to debug and understand.

utsabc · 2022-10-28T09:39:24Z

It looks good, but is this something that can be done from destination definitions itself? Why keep config at different places?

Ideally yes for a quick fix I went with this appraoch

is this going to be tech debt again? I prefer to have this to be defined at destDef level so that it is easy to debug and understand.

Make sense I will try to do an impl

…iltering via destination definition

utsabc · 2022-10-31T03:45:00Z

It looks good, but is this something that can be done from destination definitions itself? Why keep config at different places?

Ideally yes for a quick fix I went with this appraoch

is this going to be tech debt again? I prefer to have this to be defined at destDef level so that it is easy to debug and understand.

Make sense I will try to do an impl

@koladilip I have approached the problem with the way you suggested, taking the filter keys from destination definition

…ions

koladilip · 2022-10-31T03:51:52Z

processor/processor.go

-					sent to transformer for such destination. */
-					if misc.Contains(customDestinations, *destType) {
-						shallowEventCopy.Destination.Config = nil
+					configsToFilter := destination.DestinationDefinition.ConfigFilters


What happens to range operation if this is nil?

ConfigFilters won't be nil as its type Array when initialized it will be an empty array. But we can do a nil check before iterating

Lint is marking it as not needed

Yes looks like nil is handled by range so we don't need to worry about: https://play.golang.com/p/iymc4sfOdcw

…efinitionT and checks before accessing values

koladilip · 2022-10-31T08:12:21Z

processor/processor.go

-					sent to transformer for such destination. */
-					if misc.Contains(customDestinations, *destType) {
-						shallowEventCopy.Destination.Config = nil
+					configsToFilter := destination.DestinationDefinition.ConfigFilters


Yes looks like nil is handled by range so we don't need to worry about: https://play.golang.com/p/iymc4sfOdcw

…ub.com:rudderlabs/rudder-server into feat.support-for-config-to-custom-destinations

processor/processor.go

…efinitionT and checks before accessing values

…omments handled type checks

…s for added changes

utsabc · 2022-11-01T15:57:04Z

Is it possible to add tests?

Added unit test cases

…ions

atzoum · 2022-11-01T17:51:13Z

processor/processor.go

+
+func filterConfig(eventCopy *transformer.TransformerEventT, destination *backendconfig.DestinationT) {
+	if configsToFilterI, ok := destination.DestinationDefinition.Config["configFilters"]; ok {
+		if configsToFilter, ok := configsToFilterI.([]string); ok {


In practice, casting to []string has no chances for success, since this config will be populated through json unmarshalling.
https://go.dev/play/p/BLqiaLRfgO-

Please adapt relevant tests to use proper, unmarshalled configs

Make sure to typecheck the next casting that will be introduced too (k, ok := configKey.(string); ok) as a slice of any type can literally hold any type

Got it addressed the same, added unit test cases as well

…to consider array of interface and casting individual types

atzoum · 2022-11-02T06:22:43Z

processor/processor.go

@@ -509,7 +508,7 @@ func loadConfig() {
 	config.RegisterBoolConfigVariable(false, &enableEventSchemasAPIOnly, true, "EventSchemas.enableEventSchemasAPIOnly")
 	config.RegisterIntConfigVariable(10000, &maxEventsToProcess, true, 1, "Processor.maxLoopProcessEvents")

-	batchDestinations, customDestinations = misc.LoadDestinations()
+	batchDestinations, _ = misc.LoadDestinations()


since misc.LoadDestinations is only used here and we don't really care about custom destinations, please simplify it by renaming it to misc.BatchDestinations and only return a single value

Suggested change

batchDestinations, _ = misc.LoadDestinations()

batchDestinations = misc.BatchDestinations()

atzoum · 2022-11-02T06:28:21Z

processor/processor_test.go

+					Config: map[string]interface{}{
+						"configFilters": []interface{}{"long_config1", "long_config2"},
+					},


At least once we should use the outcome of json.Unmarshal in our tests, by unmarshalling a json string.

…dding test case with unmarshled configs

fracasula

@utsabc linting was not passing when you merged. can you please open a PR to fix the linting warnings? https://github.com/rudderlabs/rudder-server/actions/runs/3375866788/jobs/5602936742

utsabc · 2022-11-02T12:46:41Z

@utsabc linting was not passing when you merged. can you please open a PR to fix the linting warnings? https://github.com/rudderlabs/rudder-server/actions/runs/3375866788/jobs/5602936742

Sure

feat(support for config to custom destinations): adding support for p…

0941d2f

…assing config to transformer for custom destinations

github-actions bot added the server-team label Oct 27, 2022

utsabc requested review from atzoum, lvrach, chandumlg, cisse21, fracasula, koladilip and krishna2020 October 27, 2022 11:02

utsabc added the Ready for Review label Oct 27, 2022

utsabc self-assigned this Oct 27, 2022

koladilip reviewed Oct 27, 2022

View reviewed changes

utsabc changed the title ~~feat: support for config to custom destinations):~~ feat: support for config to custom destinations Oct 28, 2022

krishna2020 reviewed Oct 28, 2022

View reviewed changes

processor/processor.go Show resolved Hide resolved

krishna2020 mentioned this pull request Oct 28, 2022

feat: added multitopic support for kafka rudderlabs/rudder-transformer#1488

Merged

10 tasks

feat(support for config to custom destinations): adding support for f…

754f2f3

…iltering via destination definition

Merge branch 'master' into feat.support-for-config-to-custom-destinat…

c5c7898

…ions

koladilip reviewed Oct 31, 2022

View reviewed changes

krishna2020 approved these changes Oct 31, 2022

View reviewed changes

feat(support for config to custom destinations): updated DestinationD…

b155433

…efinitionT and checks before accessing values

koladilip approved these changes Oct 31, 2022

View reviewed changes

Merge branch 'feat.support-for-config-to-custom-destinations' of gith…

5bcecff

…ub.com:rudderlabs/rudder-server into feat.support-for-config-to-custom-destinations

chandumlg reviewed Nov 1, 2022

View reviewed changes

processor/processor.go Outdated Show resolved Hide resolved

chandumlg reviewed Nov 1, 2022

View reviewed changes

processor/processor.go Outdated Show resolved Hide resolved

utsabc added 2 commits November 1, 2022 20:51

feat(support for config to custom destinations): updated DestinationD…

3d241b6

…efinitionT and checks before accessing values

feat(support for config to custom destinations): updated for review c…

bf60abe

…omments handled type checks

chandumlg force-pushed the feat.support-for-config-to-custom-destinations branch from 8130004 to bf60abe Compare November 1, 2022 15:21

utsabc added 2 commits November 1, 2022 21:24

feat(support for config to custom destinations): added unit test case…

21a19fe

…s for added changes

feat(support for config to custom destinations): resolving conflict

febd80b

github-actions bot added the with tests label Nov 1, 2022

Merge branch 'master' into feat.support-for-config-to-custom-destinat…

f62e671

…ions

atzoum reviewed Nov 1, 2022

View reviewed changes

feat(support for config to custom destinations): updated type checks …

7af9692

…to consider array of interface and casting individual types

chandumlg approved these changes Nov 2, 2022

View reviewed changes

chore: lint clean up

557968c

atzoum reviewed Nov 2, 2022

View reviewed changes

utsabc added 2 commits November 2, 2022 13:43

feat(support for config to custom destinations): minor cleanups and a…

2a9a948

…dding test case with unmarshled configs

feat(support for config to custom destinations): resolving conflicts

b2ad586

atzoum approved these changes Nov 2, 2022

View reviewed changes

utsabc merged commit cb230b0 into master Nov 2, 2022

utsabc deleted the feat.support-for-config-to-custom-destinations branch November 2, 2022 08:43

rudder-server-bot mentioned this pull request Nov 2, 2022

chore: release 1.3.0 #2562

Closed

$fracasula$

fracasula reviewed Nov 2, 2022

View reviewed changes

This was referenced Nov 7, 2022

chore: release 1.3.0 #2665

Merged

chore: prerelease 1.3.0-rc.1 #2666

Merged

chore: prerelease 1.3.0-rc.2 #2673

Merged

chore: prerelease 1.3.0-rc.3 #2681

Merged

chore: release 1.3.0 #2695

Closed

chore: prerelease 1.4.0-rc.1 #2696

Closed

This was referenced Dec 1, 2022

chore: release 1.3.0 #2749

Closed

chore: prerelease 1.4.0-rc.1 #2750

Merged

chore: release 1.4.0 #2752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for config to custom destinations #2625

feat: support for config to custom destinations #2625

utsabc commented Oct 27, 2022

utsabc commented Oct 27, 2022

codecov bot commented Oct 27, 2022 •

edited

Loading

koladilip left a comment

utsabc commented Oct 27, 2022

koladilip commented Oct 28, 2022

utsabc commented Oct 28, 2022

utsabc commented Oct 31, 2022

koladilip Oct 31, 2022

utsabc Oct 31, 2022

utsabc Oct 31, 2022 •

edited

Loading

koladilip Oct 31, 2022

koladilip Oct 31, 2022

utsabc commented Nov 1, 2022

atzoum Nov 1, 2022

utsabc Nov 1, 2022

atzoum Nov 2, 2022

utsabc Nov 2, 2022

atzoum Nov 2, 2022

utsabc Nov 2, 2022

$@fracasula$ fracasula left a comment

utsabc commented Nov 2, 2022

	batchDestinations, _ = misc.LoadDestinations()
	batchDestinations = misc.BatchDestinations()

feat: support for config to custom destinations #2625

feat: support for config to custom destinations #2625

Conversation

utsabc commented Oct 27, 2022

Description

Notion Ticket

Security

utsabc commented Oct 27, 2022

codecov bot commented Oct 27, 2022 • edited Loading

Codecov Report

koladilip left a comment

Choose a reason for hiding this comment

utsabc commented Oct 27, 2022

koladilip commented Oct 28, 2022

utsabc commented Oct 28, 2022

utsabc commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utsabc Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utsabc commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fracasula left a comment

Choose a reason for hiding this comment

utsabc commented Nov 2, 2022

codecov bot commented Oct 27, 2022 •

edited

Loading

utsabc Oct 31, 2022 •

edited

Loading

$@fracasula$ fracasula left a comment