-
Notifications
You must be signed in to change notification settings - Fork 10.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Collect new statistics for the Contact Identification feature #33895
Conversation
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 8ba58ce The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #33895 +/- ##
===========================================
- Coverage 75.81% 75.81% -0.01%
===========================================
Files 512 512
Lines 22208 22221 +13
Branches 5404 5407 +3
===========================================
+ Hits 16837 16846 +9
- Misses 4720 4722 +2
- Partials 651 653 +2
Flags with carried forward coverage won't be shown. Click here to find out more. |
a738619
to
0668978
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving on behalf of frontend!
apps/meteor/app/statistics/server/lib/getContactVerificationStatistics.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/statistics/server/lib/getContactVerificationStatistics.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/statistics/server/lib/getContactVerificationStatistics.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/statistics/server/lib/getContactVerificationStatistics.ts
Outdated
Show resolved
Hide resolved
is it asking too much to create some data, perform some analysis of the queries and attach to the PR? I mean, I wonder if we should start being more cautions adding more metrics, specially when talking about omnichannel metrics as we have some HUGE datasets out there |
27d069f
to
f49e5da
Compare
* fix: Invalid association between visitors and contacts. * types * types * some tests * removed pointless tests * fix api test * uneeded import * fixed some tests * fixed some tests * fixing tests * added tests with different invalid associations * do not migrate visitors with no rooms * tests * tests * Update apps/meteor/ee/app/livechat-enterprise/server/api/contacts.ts Co-authored-by: Rafael Tapia <rafael.tapia@rocket.chat> * no need for array * adjusting tests --------- Co-authored-by: Rafael Tapia <rafael.tapia@rocket.chat>
fed388f
to
b2dee15
Compare
…nto feat/add-contact-identification-stats
Done! I wrote the results of my tests in the PR description |
…nto feat/add-contact-identification-stats
dbf9ff3
Co-authored-by: Diego Sampaio <chinello@gmail.com>
Proposed changes (including videos or screenshots)
Added statistics related to the new Contact Identification feature:
totalContacts
: Total number of contacts;totalUnknownContacts
: Total number of unknown contacts;totalMergedContacts
: Total number of merged contacts (only contacts that have been deleted on merge are considered merged -- eg if one contact has other 2 similar ones to be merged in it, we'll count 2 merged contacts, NOT 3);totalConflicts
: Total number of merge conflicts;totalResolvedConflicts
: Total number of resolved conflicts;totalBlockedContacts
: Total number of blocked contacts;totalPartiallyBlockedContacts
: Total number of contacts partially blocked -- only contacts that have at least one blocked contact (but not all of them) are counted;totalFullyBlockedContacts
: Total number of contacts fully blocked;totalVerifiedContacts
: Total number of verified contacts;avgChannelsPerContact
: Average number of channels per contact;totalContactsWithoutChannels
: Number of contacts without channels;totalImportedContacts
: Total number of imported contacts;totalUpsellViews
: Total number of "Advanced Contact Management" Upsell CTA views;totalUpsellClicks
: Total number of "Advanced Contact Management" Upsell CTA clicks;Issue(s)
Steps to test or reproduce
Further comments
For load test purposes, I populated my local DB with +2M contacts (2069065 contacts exactly) using this filler repo which I extended in a fork so that the contacts collection could also be populated randomly considering all of its optional fields and arrays of random sizes. Using this DB, I ran each new query 10 times and calculated the average execution time (in milliseconds) and a 95% confidence interval for each of them (of course, with this we're making an assumption that the average execution time for each query follow a normal distribution):
totalConflicts
andavgChannelsPerContact
(they're calculated together): 3,444.1 ±75.577 (±2.19%);totalUnknownContacts
: 324.6 ±6.268 (±1.93%);totalBlockedContacts
: 589 ±18.433 (±3.13%);totalFullyBlockedContacts
: 2,743.6 ±41.011 (±1.49%);totalPartiallyBlockedContacts
: infered fromtotalBlockedContacts
andtotalFullyBlockedContacts
;totalVerifiedContacts
: 610.7 ±23.301 (±3.82%);totalContactsWithoutChannels
: 819.9 ±23.373 (±2.85%);All new queries used a index, except
totalContactsWithoutChannels
,totalConflicts
andavgChannelsPerContact
which all need to read all documents (so I believe they're all essentially a COLSCAN).SCI-78