-
Notifications
You must be signed in to change notification settings - Fork 69
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
[BD-46] feat: added Segment track for CLI scripts #2617
[BD-46] feat: added Segment track for CLI scripts #2617
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! When this pull request is ready, tag your edX technical lead. |
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
646851c
to
c35e0c5
Compare
Waiting #2609 |
@@ -0,0 +1,22 @@ | |||
const axios = require('axios'); |
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.
Since we use paragon CLI
in production axios
it is required to have in dependencies
instead of devDependencies
.
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.
Done
bin/paragon-scripts.js
Outdated
const executor = COMMANDS[command]; | ||
|
||
sendTrackInfo(command, 'trackCLICommands'); |
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 would track only for specific command
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.
These functions will differ only in the transmitted parameter, otherwise, the logic will remain unchanged. I think it's better to reuse the code and not duplicate it
584a448
to
f02a941
Compare
f02a941
to
cb9dcb8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #2617 +/- ##
==========================================
+ Coverage 92.82% 93.28% +0.46%
==========================================
Files 235 215 -20
Lines 4237 3635 -602
Branches 1029 898 -131
==========================================
- Hits 3933 3391 -542
+ Misses 300 239 -61
- Partials 4 5 +1 ☔ View full report in Codecov by Sentry. |
const { v4: uuidv4 } = require('uuid'); | ||
const Analytics = require('analytics-node'); | ||
|
||
const analytics = new Analytics(process.env.SEGMENT_KEY); | ||
|
||
exports.handler = async function eventHandler(event) { | ||
// Only allow POST | ||
if (event.httpMethod !== 'POST') { | ||
return { statusCode: 405, body: 'Method Not Allowed' }; | ||
} | ||
const { eventName } = JSON.parse(event.body); | ||
// dispatch event to Segment | ||
analytics.track({ | ||
anonymousId: uuidv4(), | ||
event: 'openedx.paragon.functions.track-cli-commands.run', | ||
properties: { eventName }, | ||
}); | ||
|
||
return { | ||
statusCode: 200, | ||
body: JSON.stringify({ success: true }), | ||
}; | ||
}; |
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 don't think you need to create another netlify function here, instead we should modify the one we have, because they do the same thing.
I would do the following
- Rename this file to
sendAnalyticsData.js
and modify the function as follows:
exports.handler = async function eventHandler(event) {
// Only allow POST
if (event.httpMethod !== 'POST') {
return { statusCode: 405, body: 'Method Not Allowed' };
}
const { eventId, properties } = JSON.parse(event.body);
// dispatch event to Segment
analytics.track({
anonymousId: uuidv4(),
event: eventId,
properties,
});
return {
statusCode: 200,
body: JSON.stringify({ success: true }),
};
};
- Keep
trackGenerateComponent.js
in order not to make a breaking change, old version of Paragon will still send data to this function, so wee need to keep this functionality for now. Although, we probably could reuse our new handler here to avoid code duplication, something like this (not sure if it will work):
const { handler: actualHandler } = require('./sendAnalyticsData');
exports.handler = async function eventHandler(event) {
const body = JSON.parse(event.body);
event.body = JSON.stringify({
...body,
eventId: 'openedx.paragon.functions.track-generate-component.created',
properties: { componentName: body.componentName }
})
return actualHandler(event);
};
- Modify
sendTrackInfo
function to
function sendTrackInfo(eventId, properties) {
const { BASE_URL, TRACK_ANONYMOUS_ANALYTICS } = process.env;
if (TRACK_ANONYMOUS_ANALYTICS) {
const url = `${BASE_URL}/.netlify/functions/sendTrackData`;
axios.post(url, { eventId, properties })
.then(result => {
// eslint-disable-next-line no-console
console.log(`Track info is successfully sent (status ${result.status})`);
}).catch(error => {
// eslint-disable-next-line no-console
console.log(`Track info request failed (${error})`);
});
}
}
Then you could easily use this one function for any event you want, e.g. to track component generation you would do:
sendTrackInfo('openedx.paragon.functions.track-generate-component.created', { componentName });
to track CLI commands:
sendTrackInfo('openedx.paragon.cli-command.used', { command });
basically it will be able to handle any event you want in the future also without any modifications.
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.
Done
cb9f70d
to
9787ccb
Compare
bin/paragon-scripts.js
Outdated
@@ -173,6 +174,8 @@ const COMMANDS = { | |||
const [command, ...commandArgs] = process.argv.slice(2); | |||
const executor = COMMANDS[command]; | |||
|
|||
sendTrackInfo('openedx.paragon.cli-command.used', { command }); |
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.
Hmm, I think it might be a good idea to add additional info about the command, for example if it succeeded or not. What do you think about moving this code into try / catch block below and sending status of execution?
Something like:
try {
await executor.executor(commandArgs);
sendTrackInfo('openedx.paragon.cli-command.used', { command, status: 'success' });
} catch (error) {
// eslint-disable-next-line no-console
console.error(chalk.red.bold('An error occurred:', error.message));
sendTrackInfo('openedx.paragon.cli-command.used', { command, status: 'error', errorMsg: error.message });
process.exit(1);
}
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.
added, thanks
@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 22.0.0-alpha.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 23.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* feat: added Segment track for CLI scripts * refactor: refactoring after review * refactor: refactoring after review * refactor: refactoring after review * refactor: code refactoring
* feat: added Segment track for CLI scripts * refactor: refactoring after review * refactor: refactoring after review * refactor: refactoring after review * refactor: code refactoring
* feat: added Segment track for CLI scripts * refactor: refactoring after review * refactor: refactoring after review * refactor: refactoring after review * refactor: code refactoring
* feat: added Segment track for CLI scripts * refactor: refactoring after review * refactor: refactoring after review * refactor: refactoring after review * refactor: code refactoring
🎉 This PR is included in version 23.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist