-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Adding Analytics logging for JSRefactor, Live Preview, Quick Edit and more features #14253
Adding Analytics logging for JSRefactor, Live Preview, Quick Edit and more features #14253
Conversation
@@ -96,7 +96,8 @@ define(function LiveDevelopment(require, exports, module) { | |||
StringUtils = require("utils/StringUtils"), | |||
UserServer = require("LiveDevelopment/Servers/UserServer").UserServer, | |||
WebSocketTransport = require("LiveDevelopment/transports/WebSocketTransport"), | |||
PreferencesManager = require("preferences/PreferencesManager"); | |||
PreferencesManager = require("preferences/PreferencesManager"), | |||
HealthLogger = brackets.getModule("utils/HealthLogger"); |
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.
Please use require here
"usage", | ||
"livePreview", | ||
"open", | ||
"" |
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.
This is an optional param no need to send empty string.
/** | ||
* Send analytics data for Quick Doc "readMore" action | ||
* | ||
* @return {type} [[Description]] |
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.
fill type and Description field in doc comment.
} | ||
if (eventName) { | ||
// Send analytics data for refactoring | ||
HealthLogger.sendAnalyticsData( |
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.
We are not even checking whether we are in js context or not? These commands are available in all type of files. Do we just want capture click on the menu or successful usage of this feature?
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 the check for JS context.
@@ -196,6 +197,14 @@ define(function (require, exports, module) { | |||
return null; | |||
} | |||
|
|||
//Send analytics data for Quick Edit open | |||
HealthLogger.sendAnalyticsData( |
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.
Again do we are just capturing quick edit performed. Are we interested in errors which we get in this operation?
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.
we just capture logging when quick edit is performed.
// Send analytics data for Quick Doc open | ||
HealthLogger.sendAnalyticsData( | ||
"cssQuickDoc", | ||
"usage", |
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.
2nd param is event category? usage might not be the correct word. feature name should be there something like quickedit, multicursor, refactor etc
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.
@SnchitGrover would be able to clarify this.
Just one more question @sobisht |
@navch Since unit tests will call the entry functions. Logging will be sent for the unit tests also. This item will be taken care in different PR. |
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.
Thanks @sobisht for this PR. I have restarted the build.
This PR is related to adding analytics log for following features in brackets:
QuickEdit: open
QuickDoc:
Live Preview: Open
Project Settings: Edit in url
MultiCursor: Use
ping @swmitra @navch