-
Notifications
You must be signed in to change notification settings - Fork 3
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: modify segment event to remove message content #70
base: main
Are you sure you want to change the base?
feat: modify segment event to remove message content #70
Conversation
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.
A quick question.
@@ -46,7 +46,6 @@ export function addChatMessage(role, content, courseId, promptExperimentVariatio | |||
user_id: userId, | |||
timestamp: message.timestamp, | |||
role: message.role, | |||
content: message.content, |
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.
Pretty straightforward and easy change, but I'm not sure how to test Segment locally? Is this possible? Do I need to merge/deploy to stage and test there?
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.
I think there are two things to test.
- That we no longer call
sendTrackEvent
with thecontent
key in the event.- I think that this can be accomplished in testing or local development.
- That the
content
key in the event does not appear in Segment events.- I think that this can be accomplished in stage or production.
Testing
One thing I think we should consider is adding a test suite thunks.test.js to test this thunk. As part of that test, we could mock out the sendTrackEvent
function and verify that it's not called with content
. I don't think you'd need to test the entire thunk, since it predates this change. That's one way to verify that your change worked at a unit test level. Here is an existing example.
In terms of testing this additionally, I think we have some options, and it depends on what you want to test.
Local Development
This would be another form of asserting 1 above.
By default, when you call initialize in the Learning MFE to initialize the app, it'll set up the SegmentAnalyticsService here.
I'm actually not sure how that class functions locally, but I would expect that the call to sendTrackEvent
will return early here when running the Learning MFE locally, so the execution of sendTrackEvent
wouldn't even get far enough to validate the contents of the event. But it may be worth throwing a breakpoint in to see if it does get to sendTrackEvent
.
If not, we could consider overriding the analytics service via the Javascript configuration using the analyticsService key. The Javascript configuration file can be defined locally; see frontend-plugin-advertisements for an example. Here is an example of us overriding the logging
utility in stage. My approach would be to copy the MockAnalyticsService class and replace the calls to Jest
with console.log
statements or debugger
statements.
That said, this won't tell you anything about the Segment side of things, because that's not being called locally. It would pretty much just give you the same benefit as the testing - that is, asserting sendTrackEvent
was called without content
- without needing to write tests. But I think the test would probably be more valuable.
For asserting 2 above, we can test in stage and/or production.
Stage
We do seem to have a SEGMENT_KEY
set up in the stage Learning MFE. However, I'm not sure where those events go and whether we can look at them. I'll ask around!
Production
Events sent to Segment in production can be viewed in Snowflake for verification. As you said, this is a very straightforward change, so I think the risk associated with letting this go to production without validation in stage is very low. But it's not zero. If you'd feel more comfortable verifying in stage first, let's see if we can check the stage Segment events somehow.
My recommendation would be throw in a quick Jest test for the call to sendTrackEvent
from this thunk and to validate in stage. If it's not possible to validate in stage, I would be comfortable with testing in production, but I'd like other's opinions as well.
I hope this was helpful. Let me know what you think!
a6f99ef
to
6d2a19b
Compare
COSMO-547
Modify segment event to remove message contents and PII.