-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add telemetry to Todo #101
Conversation
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
=========================================
- Coverage 3.81% 3.55% -0.26%
=========================================
Files 9 10 +1
Lines 971 1042 +71
=========================================
Hits 37 37
- Misses 933 1004 +71
Partials 1 1
Continue to review full report at Codecov.
|
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.
LGTM.
We should update the Telemetry docs to indicate what is tracked for this plugin. |
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.
LGTM. A few questions bellow, but nothing blocking.
Properties map[string]interface{} | ||
} | ||
|
||
func (p *Plugin) handleTelemetry(w http.ResponseWriter, r *http.Request) { |
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.
IIRC, in the core product the client does the requests the telemetry platform itself. Is is something we should adopt for plugins?
If not, can we move this code into the library as well?
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.
My point is not sharing the key to the clients (my own concern, probably not a real concern) and to keep one single "source of truth" (the server). Also, it helps to keep some privacy for the clients, since other information (that might be useful or not) like carrier or so that probably is automatically fetched by rudder will not be fetched.
0/5 on whether or how this should be added to the library. We could add the whole handling like we are doing for the settings panel, or we can just add the handleTelemetry function and leave the routing to the plugin.
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.
Personally, I like it with the server doing the reporting and one less moving part for people to track and more consistent across plugins.
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.
Good points @larkox 👍
Exporting handleTelemetry
seams like a good first step until we figured out, what other things we need in the library.
@@ -219,6 +271,7 @@ func (p *Plugin) handleList(w http.ResponseWriter, r *http.Request) { | |||
lt := time.Unix(lastReminderAt/1000, 0).In(timezone) | |||
if nt.Sub(lt).Hours() >= 1 && (nt.Day() != lt.Day() || nt.Month() != lt.Month() || nt.Year() != lt.Year()) { | |||
p.PostBotDM(userID, "Daily Reminder:\n\n"+issuesListToString(issues)) | |||
p.trackDailySummary(userID) |
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.
What is the value we get from tracking this event?
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.
Understanding how many daily summaries are being sent. May allow us to see errors in the code (more than one daily summary per user), or see how active are the users of the plugin.
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 it makes sense to add this because we now have the user setting to disable the reminders.
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.
LGTM 👍
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.
Tested and passed
- Tested that all events get logged
- trackSendIssue
- trackAddIssue
- trackAcceptIssue
- trackBumpIssue
- trackCommand
- trackCompleteIssue
- trackRemoveIssue
- trackFrontend (todo_frontend_rhs_add)
- Data seems accurate
- Ensured that plugin works normally when
EnableDiagnostic
is set tofalse
with no crashes
LGTM!
Summary
Add telemetry to Todo. The content of the todos is not registered.
Goals:
Track:
Ticket Link
None