Skip to content
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: track gen-ai use in Intercom COMPASS-7116 #5442

Merged
merged 6 commits into from
Feb 16, 2024
Merged

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Feb 13, 2024

https://jira.mongodb.org/browse/COMPASS-7116

Description

It seems feasible that the intercom track would be reused in the future, so I added it to utils - let me know if that's not the best place. The intercom setup is currently in the main compass package.
Also please consider if I missed any other place where NL is used 😅

The new tests for GenerativeAIInput are regression, I couldn't test the new behaviour because I couldn't put a spy on intercomTrack (not without moving it directly to the utils/index file or somewhere else completely).

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

There are several logs associated with intercom setup. Do we want to log this event as well?

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the feat label Feb 13, 2024
@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Feb 13, 2024
@paula-stacho paula-stacho force-pushed the COMPASS-7116 branch 3 times, most recently from 4fea1eb to efa2d57 Compare February 14, 2024 12:13
@paula-stacho paula-stacho marked this pull request as ready for review February 14, 2024 12:13
@paula-stacho
Copy link
Contributor Author

adding a wip label as I'm addressing some comments from @mcasimir

@paula-stacho
Copy link
Contributor Author

@lerouxb added some changes after your review - a catch with a debug log, and an extra check ensuring we don't pollute external (Atlas) Intercom.

another suggestion from @mcasimir was to move the all the intercom code together. I will do this as a follow up in a separate PR

try {
win.Intercom('track', event, metadata);
} catch (error) {
debug('intercom track error', error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI: I don't think debug() would end up in a log file. If you're interested in seeing this in logs if it happens you would have to use log() on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I thought it'd make sense to use the same level as when the intercom fails to load ->

.catch((e) => debug('queue error', e));
.
but tbh I'm not sure why that level was chosen in that instance either 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably because it's not an user facing failure, so noone would look into the logs unless debugging anyway? or are there other cases where you'd imagine we'd need this in the logs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with debug() is that you can only see it if you have debug enabled and you're looking at the console or stdout. So it isn't something a user can spot in a log file and send it to us and we'd have to already know how to recreate the intercom failure in order to generate any debug output locally.

Which might be fine. Just thought I'd bring it up.

@paula-stacho paula-stacho merged commit 946f648 into main Feb 16, 2024
16 checks passed
@paula-stacho paula-stacho deleted the COMPASS-7116 branch February 16, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants