Skip to content

feat: Collect new statistics for the Contact Identification feature#33895

Merged
kodiakhq[bot] merged 22 commits intodevelopfrom
feat/add-contact-identification-stats
Dec 20, 2024
Merged

feat: Collect new statistics for the Contact Identification feature#33895
kodiakhq[bot] merged 22 commits intodevelopfrom
feat/add-contact-identification-stats

Conversation

@matheusbsilva137
Copy link
Contributor

@matheusbsilva137 matheusbsilva137 commented Nov 6, 2024

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 and avgChannelsPerContact (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 from totalBlockedContacts and totalFullyBlockedContacts;
  • totalVerifiedContacts: 610.7 ±23.301 (±3.82%);
  • totalContactsWithoutChannels: 819.9 ±23.373 (±2.85%);

All new queries used a index, except totalContactsWithoutChannels, totalConflicts and avgChannelsPerContact which all need to read all documents (so I believe they're all essentially a COLSCAN).

SCI-78

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 6, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Nov 6, 2024

🦋 Changeset detected

Latest commit: 8ba58ce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/core-typings Minor
@rocket.chat/model-typings Minor
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/rest-typings Minor
@rocket.chat/ui-contexts Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-video-conf Major
@rocket.chat/ui-voip Major
@rocket.chat/web-ui-registration Major
@rocket.chat/instance-status Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://RocketChat.github.io/Rocket.Chat/pr-preview/pr-33895/
on branch gh-pages at 2024-12-19 23:08 UTC

@matheusbsilva137 matheusbsilva137 added this to the 7.1.0 milestone Nov 6, 2024
@codecov
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 75.81%. Comparing base (76f6239) to head (8ba58ce).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             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     
Flag Coverage Δ
unit 75.81% <75.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@dougfabris dougfabris force-pushed the feat/single-contact-id branch from a738619 to 0668978 Compare November 6, 2024 17:24
@matheusbsilva137 matheusbsilva137 marked this pull request as ready for review November 6, 2024 19:10
@matheusbsilva137 matheusbsilva137 requested review from a team as code owners November 6, 2024 19:10
dougfabris
dougfabris previously approved these changes Nov 7, 2024
Copy link
Member

@dougfabris dougfabris left a 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!

@sampaiodiego
Copy link
Member

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

@dougfabris dougfabris force-pushed the feat/single-contact-id branch from 27d069f to f49e5da Compare November 13, 2024 22:03
Base automatically changed from feat/single-contact-id to develop November 19, 2024 19:30
@ggazzo ggazzo dismissed dougfabris’s stale review November 19, 2024 19:30

The base branch was changed.

@ggazzo ggazzo requested review from a team as code owners November 19, 2024 19:30
@renatobecker renatobecker modified the milestones: 7.1.0, 7.2.0 Nov 25, 2024
@matheusbsilva137 matheusbsilva137 marked this pull request as draft November 25, 2024 22:42
matheusbsilva137 and others added 6 commits November 27, 2024 10:42
* 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>
@matheusbsilva137 matheusbsilva137 removed request for a team November 27, 2024 15:43
@matheusbsilva137
Copy link
Contributor Author

is it asking too much to create some data, perform some analysis of the queries and attach to the PR?

Done! I wrote the results of my tests in the PR description

@matheusbsilva137 matheusbsilva137 marked this pull request as ready for review December 2, 2024 14:26
MarcosSpessatto
MarcosSpessatto previously approved these changes Dec 4, 2024
KevLehman
KevLehman previously approved these changes Dec 4, 2024
…nto feat/add-contact-identification-stats
Co-authored-by: Diego Sampaio <chinello@gmail.com>
@scuciatto scuciatto added the stat: QA assured Means it has been tested and approved by a company insider label Dec 19, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Dec 19, 2024
@kodiakhq kodiakhq bot merged commit 2e4af86 into develop Dec 20, 2024
@kodiakhq kodiakhq bot deleted the feat/add-contact-identification-stats branch December 20, 2024 00:24
MartinSchoeler pushed a commit that referenced this pull request Dec 30, 2024
…33895)

Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
Co-authored-by: Douglas Fabris <27704687+dougfabris@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants