feat: enrich with geo location data during destination transformation #2803

sanpj2292 · 2023-11-07T10:37:32Z

Description of the change

We are enriching the event payload with geo-location data just before destination transformation

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Resolves INT-1000
Resolved INT-1001

Checklists

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
"Ready for review" label attached to the PR and reviewers mentioned in a comment
Changes have been reviewed by at least one other engineer
Issue from task tracker has a link to this pull request

Signed-off-by: Sai Sankeerth <[email protected]>

…to feat.enrich-geo-location

Signed-off-by: Sai Sankeerth <[email protected]>

codecov · 2023-11-07T11:54:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a6f7d13) 87.11% compared to head (a2f0747) 87.13%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2803      +/-   ##
===========================================
+ Coverage    87.11%   87.13%   +0.02%     
===========================================
  Files          768      770       +2     
  Lines        28653    28714      +61     
  Branches      6726     6740      +14     
===========================================
+ Hits         24960    25021      +61     
  Misses        3350     3350              
  Partials       343      343

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

Signed-off-by: Sai Sankeerth <[email protected]>

- add router transformation test-case - change test-case for helper for first-key found logic Signed-off-by: Sai Sankeerth <[email protected]>

Signed-off-by: Sai Sankeerth <[email protected]>

…ints & asynchronise geolocation enricher Signed-off-by: Sai Sankeerth <[email protected]>

…to feat.enrich-geo-location

src/helpers/geoLocation.ts

src/middlewares/enricher.ts

src/helpers/geoLocation.ts

src/v0/util/index.js

src/helpers/geoLocation.ts

- simplify logic for finding addressKey - update name of first matching key-value pair finder function - simplify logic for finding first key-value pair - add validation on message to be an object to perform search operation Signed-off-by: Sai Sankeerth <[email protected]>

Signed-off-by: Sai Sankeerth <[email protected]>

…tead of identifying the valid values manually, added test-cases Signed-off-by: Sai Sankeerth <[email protected]>

Signed-off-by: Sai Sankeerth <[email protected]>

…null message cases Signed-off-by: Sai Sankeerth <[email protected]>

krishna2020 · 2023-11-09T15:36:38Z

src/types/index.ts

+  properties: object;
+  traits: object;
+  [key: string]: FixMe;
+}>;

 type RouterTransformationRequestData = {


This is low level struct used in both process & routerTransform flow, so shouldn't the name be neutral ?

Suggested change

type RouterTransformationRequestData = {

type EventRequestData = {

Basically it was named as RouterTransformationRequestData previously but since the elements in the type were common, I used it as a low-level struct.

Tried to change name, there are many files which are using this type. I feel this is a little bigger change & out of scope of this task. May be we can handle this in a backlog item and make a change independent of this task. But yeah, let me know if we are ok with making this change as part of this PR.

krishna2020 · 2023-11-09T15:39:32Z

src/types/index.ts

+  | 'alias'
+  | 'audiencelist';
+
+type RudderMessageSchema = Partial<{


there's RudderMessage too -

rudder-transformer/src/types/index.ts

Line 178 in e4c6bb2

type RudderMessage = {

Do we need both of them ?

I used RudderMessage itself but there were too many changes that were needed in order for the tests to pass & compile. So, kind of yes, we need both of them.
[Backlog] We have to plan to get rid of both of these types & stick with only one type.

krishna2020 · 2023-11-09T15:52:02Z

test/middleware/enricher.test.ts

+          receivedAt: '2020-04-17T20:12:44.758+05:30',
+          request_ip: '[::1]:53513',
+          sentAt: '2020-04-17T14:42:44.722Z',
+          traits: {


Is the test case meant to have address but not context.geo ?
Next test case is exactly same

Fixed the case to have address but not context.geo

krishna2020 · 2023-11-09T15:53:24Z

test/middleware/enricher.test.ts

+        message: {
+          anonymousId: 'ac7722c2-ccb6-4ae2-baf6-1effe861f4cd',
+          channel: 'web',
+          context: {


As we are not asserting other attributes & that's not the scope in unit test here, can keep the context object simple by restricting to only context.geo or empty context

Makes sense. Incorporated the suggestions

krishna2020 · 2023-11-09T15:59:33Z

test/utils/getFirstMatchingKeyAndValue.test.ts

+      key: 'rivals.spain',
+    },
+  },
+];


should we also have a test case where both 'spain.rivals', 'rivals.spain' would be present in the object ?
If the customer instruments address at both the places ? ["traits.address", "context.traits.address"]

Makes sense. Will include one such case

Signed-off-by: Sai Sankeerth <[email protected]>

…to feat.enrich-geo-location

Signed-off-by: Sai Sankeerth <[email protected]>

src/v0/util/index.js

src/helpers/geoLocation.ts

abhimanyubabbar · 2023-11-10T08:02:39Z

src/helpers/geoLocation.ts

+    return { key: addressKey, value };
+  }
+
+  public static getMessageWithGeoLocationData(


what do you think about name enrichMessageAddressWithGeoData ?

src/middlewares/enricher.ts

abhimanyubabbar · 2023-11-10T08:06:36Z

src/middlewares/enricher.ts

+  private static enrichWithGeoInfo(
+    data: RouterTransformationRequestData[],
+  ): RouterTransformationRequestData[] {
+    return data.map((inpEv) => {


acronyms like inpEv might not be readable, let's name them as input or event or inputEvent

Incorporated

- update enricher class name, method, var name & update related test-cases - return value as undefined when anything apart from Array or object is sent for searching - make use of msg rather than message Signed-off-by: Sai Sankeerth <[email protected]>

…to feat.enrich-geo-location

src/v0/util/index.js

abhimanyubabbar

Except for one last comment made, the changes to me look good.

Signed-off-by: Sai Sankeerth <[email protected]>

…to feat.enrich-geo-location

abhimanyubabbar

LGTM

…to feat.enrich-geo-location

src/v0/util/index.js

src/helpers/geoLocation.ts

src/middlewares/enricher.ts

- move DTRequest to types/index.ts - remove unnecessary optional chaining Signed-off-by: Sai Sankeerth <[email protected]>

Signed-off-by: Sai Sankeerth <[email protected]>

…to feat.enrich-geo-location

sonarqubecloud · 2023-11-14T11:15:45Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

sanpj2292 · 2023-12-01T07:30:37Z

Closing this PR as these changes are not necessary

Sai Sankeerth added 5 commits November 6, 2023 22:48

feat: geo location enrichment in destination transformations

5bdb63b

chore: add unit-tests for geoLocation logic

0f44408

Signed-off-by: Sai Sankeerth <[email protected]>

chore: provide flexibility for an optional rudderMessage schema

4bcc8db

Signed-off-by: Sai Sankeerth <[email protected]>

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

0225128

…to feat.enrich-geo-location

feat: add tests for unit-testing enrichment logic

d789f0e

Signed-off-by: Sai Sankeerth <[email protected]>

Sai Sankeerth added 5 commits November 7, 2023 18:40

fix: address and context.geo not present case

d3d6b72

Signed-off-by: Sai Sankeerth <[email protected]>

fix: logic to include geolocation info in first found key

2249441

- add router transformation test-case - change test-case for helper for first-key found logic Signed-off-by: Sai Sankeerth <[email protected]>

fix: add unit-tests for getting addressKey

6dd5c8b

Signed-off-by: Sai Sankeerth <[email protected]>

fix: remove unnecessary operation for getKeyAndValueFromMessage

53fdb76

Signed-off-by: Sai Sankeerth <[email protected]>

chore: remove unnecessary logic in getKeyAndValueFromMessage

8fb7668

Signed-off-by: Sai Sankeerth <[email protected]>

sanpj2292 marked this pull request as ready for review November 8, 2023 05:45

sanpj2292 requested review from a team and sivashanmukh as code owners November 8, 2023 05:45

sanpj2292 requested review from chandumlg and sandeepdsvs November 8, 2023 05:45

Sai Sankeerth added 2 commits November 8, 2023 19:22

feat: add geo location middleware to destination transformation endpo…

c2363d0

…ints & asynchronise geolocation enricher Signed-off-by: Sai Sankeerth <[email protected]>

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

5bff71f

…to feat.enrich-geo-location

sanpj2292 requested a review from abhimanyubabbar November 9, 2023 04:55

abhimanyubabbar requested changes Nov 9, 2023

View reviewed changes

Sai Sankeerth added 3 commits November 9, 2023 17:34

fix: name of the returned object & name of the function

a825926

Signed-off-by: Sai Sankeerth <[email protected]>

fix: addressing minor issues related to naming, used lodash.isNil ins…

b782b90

…tead of identifying the valid values manually, added test-cases Signed-off-by: Sai Sankeerth <[email protected]>

sanpj2292 self-assigned this Nov 9, 2023

sanpj2292 requested a review from abhimanyubabbar November 9, 2023 14:03

Sai Sankeerth added 2 commits November 9, 2023 20:07

fix: correcting test-case

687f2fa

Signed-off-by: Sai Sankeerth <[email protected]>

fix: cloneDeep for all, add test-cases to reflect same referencing & …

e4c6bb2

…null message cases Signed-off-by: Sai Sankeerth <[email protected]>

krishna2020 reviewed Nov 9, 2023

View reviewed changes

Sai Sankeerth added 2 commits November 9, 2023 22:20

fix: address comments in test-cases

2c44999

Signed-off-by: Sai Sankeerth <[email protected]>

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

ee08e4b

…to feat.enrich-geo-location

sanpj2292 requested a review from krishna2020 November 9, 2023 17:40

fix: unnecessary test-case data for enricher unit-tests

8928020

Signed-off-by: Sai Sankeerth <[email protected]>

abhimanyubabbar requested changes Nov 10, 2023

View reviewed changes

Sai Sankeerth added 2 commits November 10, 2023 14:50

fix: addressing comments - 2

25fb0d7

- update enricher class name, method, var name & update related test-cases - return value as undefined when anything apart from Array or object is sent for searching - make use of msg rather than message Signed-off-by: Sai Sankeerth <[email protected]>

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

9c9c9f3

…to feat.enrich-geo-location

abhimanyubabbar reviewed Nov 10, 2023

View reviewed changes

src/v0/util/index.js Outdated Show resolved Hide resolved

abhimanyubabbar previously approved these changes Nov 10, 2023

View reviewed changes

fix: update the default value to be returned to be undefined

5ef41f5

Signed-off-by: Sai Sankeerth <[email protected]>

sanpj2292 dismissed abhimanyubabbar’s stale review via 5ef41f5 November 10, 2023 11:13

sanpj2292 requested a review from abhimanyubabbar November 10, 2023 11:14

Sai Sankeerth added 2 commits November 10, 2023 17:32

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

58fcdce

…to feat.enrich-geo-location

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

a61fb53

…to feat.enrich-geo-location

abhimanyubabbar previously approved these changes Nov 13, 2023

View reviewed changes

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

67aed69

…to feat.enrich-geo-location

koladilip reviewed Nov 14, 2023

View reviewed changes

src/v0/util/index.js Outdated Show resolved Hide resolved

src/helpers/geoLocation.ts Show resolved Hide resolved

src/helpers/geoLocation.ts Outdated Show resolved Hide resolved

src/helpers/geoLocation.ts Show resolved Hide resolved

src/middlewares/enricher.ts Outdated Show resolved Hide resolved

Sai Sankeerth added 2 commits November 14, 2023 15:19

fix: addressing comments - 3

3365332

- move DTRequest to types/index.ts - remove unnecessary optional chaining Signed-off-by: Sai Sankeerth <[email protected]>

fix: expanded type name & extra optional chaining removal

67f9217

Signed-off-by: Sai Sankeerth <[email protected]>

sanpj2292 dismissed abhimanyubabbar’s stale review via 67f9217 November 14, 2023 10:00

Merge branch 'develop' of github.com:rudderlabs/rudder-transformer in…

a2f0747

…to feat.enrich-geo-location

sanpj2292 requested review from koladilip and abhimanyubabbar November 17, 2023 12:40

sanpj2292 closed this Dec 1, 2023

devops-github-rudderstack deleted the feat.enrich-geo-location branch February 15, 2024 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enrich with geo location data during destination transformation #2803

feat: enrich with geo location data during destination transformation #2803

sanpj2292 commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

krishna2020 Nov 9, 2023

sanpj2292 Nov 9, 2023 •

edited

Loading

krishna2020 Nov 9, 2023

sanpj2292 Nov 9, 2023

krishna2020 Nov 9, 2023

sanpj2292 Nov 9, 2023

krishna2020 Nov 9, 2023

sanpj2292 Nov 10, 2023

krishna2020 Nov 9, 2023 •

edited

Loading

sanpj2292 Nov 9, 2023

sanpj2292 Nov 9, 2023

abhimanyubabbar Nov 10, 2023

sanpj2292 Nov 10, 2023

abhimanyubabbar Nov 10, 2023

sanpj2292 Nov 10, 2023

abhimanyubabbar left a comment

abhimanyubabbar left a comment

sonarqubecloud bot commented Nov 14, 2023

sanpj2292 commented Dec 1, 2023

	type RouterTransformationRequestData = {
	type EventRequestData = {

feat: enrich with geo location data during destination transformation #2803

feat: enrich with geo location data during destination transformation #2803

Conversation

sanpj2292 commented Nov 7, 2023 • edited Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sanpj2292 Nov 9, 2023 • edited Loading

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

Choose a reason for hiding this comment

krishna2020 Nov 9, 2023 • edited Loading

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

Choose a reason for hiding this comment

abhimanyubabbar left a comment

Choose a reason for hiding this comment

abhimanyubabbar left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 14, 2023

sanpj2292 commented Dec 1, 2023

sanpj2292 commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

sanpj2292 Nov 9, 2023 •

edited

Loading

krishna2020 Nov 9, 2023 •

edited

Loading