fix(reporting): gateway events are not reported when source is connected to no destination(or device mode destination) #1822

Sidddddarth · 2022-04-07T16:33:57Z

Description of the change

Moved around metric computation in processor to get proper ingestion numbers.

Sources with no destinations

Sources with Device mode destinations

Example scenarios:

1 source - no dest: gateway = 1, destination_filter = -1(diff)
1 source - device mode dest: gateway = 1, destination_filter = -1(diff)
1 source - 2 dests(1 device, 1 cloud): gateway = 1, destination_filter = 1, transformation = 1, router = 1

Notion Link

https://www.notion.so/rudderstacks/Reporting-does-not-report-Gateway-events-when-there-is-no-destination-is-connected-to-a-source-b5a37d7df9944c84818c42475d7c55ca

Type of change

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

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added unit tests for the code
I have made corresponding changes to the documentation

Security

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

codecov · 2022-04-07T16:55:29Z

Codecov Report

Merging #1822 (8eca515) into master (c184c24) will decrease coverage by 0.02%.
The diff coverage is 16.12%.

@@            Coverage Diff             @@
##           master    #1822      +/-   ##
==========================================
- Coverage   24.24%   24.21%   -0.03%     
==========================================
  Files         122      122              
  Lines       24236    24294      +58     
==========================================
+ Hits         5876     5884       +8     
- Misses      18010    18059      +49     
- Partials      350      351       +1

Impacted Files	Coverage Δ
utils/types/reporting_types.go	`0.00% <ø> (ø)`
processor/processor.go	`69.76% <16.12%> (-2.18%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c184c24...8eca515. Read the comment docs.

atzoum

Still many questions will go through the changes together

atzoum · 2022-04-08T09:43:09Z

processor/processor.go

 		if countMetadataMap != nil {
 			if _, ok := countMetadataMap[countKey]; !ok {
-				countMetadataMap[countKey] = MetricMetadata{sourceID: event.Metadata.SourceID, destinationID: event.Metadata.DestinationID, sourceBatchID: event.Metadata.SourceBatchID, sourceTaskID: event.Metadata.SourceTaskID, sourceTaskRunID: event.Metadata.SourceTaskRunID, sourceJobID: event.Metadata.SourceJobID, sourceJobRunID: event.Metadata.SourceJobRunID, sourceDefinitionID: event.Metadata.SourceDefinitionID, destinationDefinitionID: event.Metadata.DestinationDefinitionID, sourceCategory: event.Metadata.SourceCategory}
+				countMetadataMap[countKey] = MetricMetadata{


atzoum · 2022-04-08T09:43:18Z

processor/processor.go

@@ -879,21 +879,59 @@ func (proc *HandleT) updateMetricMaps(countMetadataMap map[string]MetricMetadata
 		var eventType string
 		eventName = event.Metadata.EventName
 		eventType = event.Metadata.EventType
-		countKey := strings.Join([]string{event.Metadata.SourceID, event.Metadata.DestinationID, event.Metadata.SourceBatchID, eventName, eventType}, METRICKEYDELIMITER)
+
+		countKey := strings.Join([]string{


processor/processor.go

atzoum · 2022-04-08T11:10:22Z

processor/processor.go

+					sourceForSingularEvent,
+				)
+
+				//REPORTING - GATEWAY metrics - START


The current implementation of capturing reporting metrics seems to be convoluted. The least I can say is that it is hard to follow, since as it appears many things need to happen at different stages of this method, without a clear purpose.
I will invest some time to study the current implementation in hope of forming a cleaner picture for the specific domain, otherwise I cannot be of any more use with regards to reviewing this PR.

Co-authored-by: Aris Tzoumas <atzoumas@rudderstack.com>

atzoum · 2022-04-09T12:40:18Z

Please update the pull request's title according to our new policy

Sidddddarth · 2022-04-09T17:35:58Z

Please update the pull request's title according to our new policy

Is there an example I can see?
Because this is how I interpreted the policy wrt the title😅

From the article:
<type>[optional scope]: <description>
I think this is a fix, although not a bug. Maybe it's a feature. Optional scope would be Reporting here?
The description portion can be shortened a bit more imo. I'll do that now.

Suggestions/corrections welcome.

atzoum · 2022-04-09T18:34:27Z

Please update the pull request's title according to our new policy

Is there an example I can see? Because this is how I interpreted the policy wrt the title😅

From the article: <type>[optional scope]: <description> I think this is a fix, although not a bug. Maybe it's a feature. Optional scope would be Reporting here? The description portion can be shortened a bit more imo. I'll do that now.

Suggestions/corrections welcome.

This should be enough (I used the title from the notion ticket, but feel free to change it if there is something more appropriate):

fix(reporting): gateway events are not reported when there is no destination connected to a source

In the title you are free to use language that humans can understand (with spaces) 😊

Sidddddarth · 2022-04-10T02:57:53Z

This should be enough (I used the title from the notion ticket, but feel free to change it if there is something more appropriate):
fix(reporting): gateway events are not reported when there is no destination connected to a source
In the title you are free to use language that humans can understand (with spaces) 😊

Understood now.
I'd taken these examples to heart:

feat.pendingEvtRedesign
fix.routerCrashLoop
chore.ghLabelAction
refactor.jobsdbQueries
exp.tzoumasa
doc.readme
test.multitenantPickupAlg
emphasis on human-readable.

reportsForNoDestSourcesWithDiff

bc45f9d

cisse21 and others added 2 commits April 8, 2022 09:41

Merge branch 'master' into fix.reportsForNoDestSources

d2dd374

followMetricsConvention

fe67c5d

atzoum reviewed Apr 8, 2022

View reviewed changes

Sidddddarth changed the title ~~reportsForNoDestSourcesWithDiff~~ fix.reportsForNoDestSourcesWithDiff Apr 8, 2022

minorCommentFix

8eca515

Co-authored-by: Aris Tzoumas <atzoumas@rudderstack.com>

Sidddddarth force-pushed the fix.reportsForNoDestSources branch from 2652bd6 to 8eca515 Compare April 8, 2022 15:06

Sidddddarth changed the title ~~fix.reportsForNoDestSourcesWithDiff~~ fix.Reporting:reportsForNoDestSourcesWithDiff Apr 8, 2022

Sidddddarth changed the title ~~fix.Reporting:reportsForNoDestSourcesWithDiff~~ fix.Reporting:introducingDESTINATIONFILTER_PU Apr 9, 2022

Sidddddarth changed the title ~~fix.Reporting:introducingDESTINATIONFILTER_PU~~ fix(reporting): gateway events are not reported when source is connected to no destination(or device mode destination) Apr 10, 2022

chandumlg approved these changes Apr 10, 2022

View reviewed changes

cisse21 approved these changes Apr 10, 2022

View reviewed changes

cisse21 merged commit 2962bd6 into master Apr 10, 2022

cisse21 deleted the fix.reportsForNoDestSources branch April 10, 2022 15:57

This was referenced Jun 10, 2022

chore(master): release 1.0.0 #2046

Closed

chore(master): release 1.0.0 #2050

Closed

This was referenced Jun 14, 2022

chore(master): release 1.0.0 #2051

Closed

chore(master): release 1.0.0 #2052

Closed

chore: release 0.1.10 #2055

Merged

This was referenced Jun 23, 2022

chore: release 0.1.10 #2098

Merged

chore: release 0.1.11 #2101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(reporting): gateway events are not reported when source is connected to no destination(or device mode destination) #1822

fix(reporting): gateway events are not reported when source is connected to no destination(or device mode destination) #1822

Sidddddarth commented Apr 7, 2022 •

edited

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading

atzoum left a comment

atzoum Apr 8, 2022

atzoum Apr 8, 2022

atzoum Apr 8, 2022

atzoum commented Apr 9, 2022

Sidddddarth commented Apr 9, 2022

atzoum commented Apr 9, 2022

Sidddddarth commented Apr 10, 2022

fix(reporting): gateway events are not reported when source is connected to no destination(or device mode destination) #1822

fix(reporting): gateway events are not reported when source is connected to no destination(or device mode destination) #1822

Conversation

Sidddddarth commented Apr 7, 2022 • edited Loading

Description of the change

Notion Link

Type of change

Checklist:

Security

codecov bot commented Apr 7, 2022 • edited Loading

Codecov Report

atzoum left a comment

Choose a reason for hiding this comment

atzoum Apr 8, 2022

Choose a reason for hiding this comment

atzoum Apr 8, 2022

Choose a reason for hiding this comment

atzoum Apr 8, 2022

Choose a reason for hiding this comment

atzoum commented Apr 9, 2022

Sidddddarth commented Apr 9, 2022

atzoum commented Apr 9, 2022

Sidddddarth commented Apr 10, 2022

Sidddddarth commented Apr 7, 2022 •

edited

Loading

codecov bot commented Apr 7, 2022 •

edited

Loading