feat: implement an interface to collect data of tracked users #4826
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4826 +/- ##
==========================================
+ Coverage 74.36% 74.40% +0.04%
==========================================
Files 418 419 +1
Lines 48998 49126 +128
==========================================
+ Hits 36435 36553 +118
- Misses 10183 10184 +1
- Partials 2380 2389 +9 ☔ View full report in Codecov by Sentry. |
| m := &migrator.Migrator{ | ||
| Handle: dbHandle, | ||
| MigrationsTable: "tracked_users_reports_migrations", | ||
| ShouldForceSetLowerVersion: config.GetBool("SQLMigrator.forceSetLowerVersion", true), | ||
| } | ||
| err = m.Migrate("tracked_users") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not run tracked_users_reports migrations: %w", err) | ||
| } |
There was a problem hiding this comment.
I would avoid running a migration as part of new.
We could instead:
- Expose a separate method for running migration. Expecting users of the method to call it.
- or use sync.once on
CollectDatato ensure it run the first time CollectData call was made, but makes error handling a bit more complicated.
We should pass a context to migration as well.
There was a problem hiding this comment.
migrator currently do not takes context as a param, I can create a new task for adding support for context while running migrations.
| @@ -0,0 +1,192 @@ | |||
| package trackedusers | |||
There was a problem hiding this comment.
[subjective] I would go with mtu for package name. As it seems the dominant name used among business and engineering to refer to this feature. It is also short and unique.
| package trackedusers | |
| package mtu |
There was a problem hiding this comment.
I prefer not to use abbreviations, as when a new person looks at the code he might not know what mtu is. Also want to avoid using monthly as we are not doing anything monthly in the code.
Wanted to keep engineering and business use cases separate as business requirements might change more frequently.
| @@ -0,0 +1,192 @@ | |||
| package trackedusers | |||
|
|
|||
| //go:generate mockgen -destination=./mocks/mock_data_collector.go -package=mockdatacollector github.com/rudderlabs/rudder-server/enterprise/trackedusers DataCollector | |||
There was a problem hiding this comment.
I would avoid using mockgen for now.
It has simple enough interface that makes it easy to create a custom mock. Usually, the generated mock is not very convenient to use, and leading to partial testing in practise.
A good mock for the MTU collector could count the number of unique IDs per workspace per anon/user/identify call. Thus instead of asserting if method was called with the correct arguments, we can test the outcome.
Also consider exposing a NOOP implementation, it would be useful for disabling the functionality and for tests that don't require testing mtu.
There was a problem hiding this comment.
in this particular using mocks makes more sense as we just want to make sure that this methods are correctly called with correct parameters in unit tests, additional logic can be tested in integration tests.
| type DataCollector interface { | ||
| CollectData(ctx context.Context, jobs []*jobsdb.JobT, tx *txn.Tx) error | ||
| } |
There was a problem hiding this comment.
[subjective]
| type DataCollector interface { | |
| CollectData(ctx context.Context, jobs []*jobsdb.JobT, tx *txn.Tx) error | |
| } | |
| type UserTracker interface { | |
| Track(ctx context.Context, tx *txn.Tx, jobs []*jobsdb.JobT) error | |
| } |
Datais a bit generic, I do prefer the more applicableUserterm.- Track sounds better than Collect, as we do keep any of the jobs rather use them to infer some data
There was a problem hiding this comment.
Track feels like we are tracking user activity
how about ?
type UsersReporter interface {
ReportUsers(ctx context.Context, reports []*UsersReport, tx *txn.Tx) error
}
Description
Implementing an interface to collect data of tracked user's userID and anonymousID. This data will used to estimate number of unique users we are tracking. We will not store the raw userIDs and anonymousIDs directly for tracking count. We will store IDs in a probabilistic data structure called HyperLogLog. which will help us to estimate the cardinality.
For some events will have userIDs where user is known and for some events user is not known so we will have anonymousID. for some unknown users earlier which are also known know we will have both userID and anonymousID. Then count of unique users tracked will be equal to no of unique userID + number of unique anonymousID - number of unique combinations of (userID+anonymousID).
Linear Ticket
pipe-1200
Security