-
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
Changes from all commits
e4b8a65
a454b75
dc763fd
914501a
4d03a08
690e269
31d7cf5
e4f13b7
71dcc26
b34b25d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,5 @@ | ||
# Include custom targets and environment variables here | ||
ifndef MM_RUDDER_WRITE_KEY | ||
MM_RUDDER_WRITE_KEY = 1d5bMvdrfWClLxgK1FvV3s4U1tg | ||
endif | ||
GO_BUILD_FLAGS += -ldflags '-X "github.com/mattermost/mattermost-plugin-api/experimental/telemetry.rudderWriteKey=$(MM_RUDDER_WRITE_KEY)"' |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
"sync" | ||
"time" | ||
|
||
"github.com/mattermost/mattermost-plugin-api/experimental/telemetry" | ||
"github.com/mattermost/mattermost-server/v5/model" | ||
"github.com/mattermost/mattermost-server/v5/plugin" | ||
"github.com/pkg/errors" | ||
|
@@ -57,6 +58,9 @@ type Plugin struct { | |
configuration *configuration | ||
|
||
listManager ListManager | ||
|
||
telemetryClient telemetry.Client | ||
tracker telemetry.Tracker | ||
} | ||
|
||
func (p *Plugin) OnActivate() error { | ||
|
@@ -77,9 +81,25 @@ func (p *Plugin) OnActivate() error { | |
|
||
p.listManager = NewListManager(p.API) | ||
|
||
p.telemetryClient, err = telemetry.NewRudderClient() | ||
if err != nil { | ||
p.API.LogWarn("telemetry client not started", "error", err.Error()) | ||
} | ||
|
||
return p.API.RegisterCommand(getCommand()) | ||
} | ||
|
||
func (p *Plugin) OnDeactivate() error { | ||
if p.telemetryClient != nil { | ||
err := p.telemetryClient.Close() | ||
if err != nil { | ||
p.API.LogWarn("OnDeactivate: failed to close telemetryClient", "error", err.Error()) | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// ServeHTTP demonstrates a plugin that handles HTTP requests by greeting the world. | ||
func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) { | ||
switch r.URL.Path { | ||
|
@@ -95,13 +115,41 @@ func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Req | |
p.handleAccept(w, r) | ||
case "/bump": | ||
p.handleBump(w, r) | ||
case "/telemetry": | ||
p.handleTelemetry(w, r) | ||
case "/config": | ||
p.handleConfig(w, r) | ||
default: | ||
http.NotFound(w, r) | ||
} | ||
} | ||
|
||
type telemetryAPIRequest struct { | ||
Event string | ||
Properties map[string]interface{} | ||
} | ||
|
||
func (p *Plugin) handleTelemetry(w http.ResponseWriter, r *http.Request) { | ||
userID := r.Header.Get("Mattermost-User-ID") | ||
if userID == "" { | ||
http.Error(w, "Not authorized", http.StatusUnauthorized) | ||
return | ||
} | ||
|
||
var telemetryRequest *telemetryAPIRequest | ||
decoder := json.NewDecoder(r.Body) | ||
err := decoder.Decode(&telemetryRequest) | ||
if err != nil { | ||
p.API.LogError("Unable to decode JSON err=" + err.Error()) | ||
p.handleErrorWithCode(w, http.StatusBadRequest, "Unable to decode JSON", err) | ||
return | ||
} | ||
|
||
if telemetryRequest.Event != "" { | ||
p.trackFrontend(userID, telemetryRequest.Event, telemetryRequest.Properties) | ||
} | ||
} | ||
|
||
type addAPIRequest struct { | ||
Message string `json:"message"` | ||
SendTo string `json:"send_to"` | ||
|
@@ -133,6 +181,7 @@ func (p *Plugin) handleAdd(w http.ResponseWriter, r *http.Request) { | |
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to add issue", err) | ||
return | ||
} | ||
p.trackAddIssue(userID, sourceWebapp, addRequest.PostID != "") | ||
replyMessage := fmt.Sprintf("@%s attached a todo to this thread", senderName) | ||
p.postReplyIfNeeded(addRequest.PostID, replyMessage, addRequest.Message) | ||
return | ||
|
@@ -152,6 +201,7 @@ func (p *Plugin) handleAdd(w http.ResponseWriter, r *http.Request) { | |
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to add issue", err) | ||
return | ||
} | ||
p.trackAddIssue(userID, sourceWebapp, addRequest.PostID != "") | ||
replyMessage := fmt.Sprintf("@%s attached a todo to this thread", senderName) | ||
p.postReplyIfNeeded(addRequest.PostID, replyMessage, addRequest.Message) | ||
return | ||
|
@@ -165,6 +215,8 @@ func (p *Plugin) handleAdd(w http.ResponseWriter, r *http.Request) { | |
return | ||
} | ||
|
||
p.trackSendIssue(userID, sourceWebapp, addRequest.PostID != "") | ||
|
||
receiverMessage := fmt.Sprintf("You have received a new Todo from @%s", senderName) | ||
p.sendRefreshEvent(receiver.Id) | ||
p.PostBotCustomDM(receiver.Id, receiverMessage, addRequest.Message, issueID) | ||
|
@@ -224,6 +276,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 commentThe 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 commentThe 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 commentThe 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. |
||
err = p.saveLastReminderTimeForUser(userID) | ||
if err != nil { | ||
p.API.LogError("Unable to save last reminder for user err=" + err.Error()) | ||
|
@@ -271,6 +324,8 @@ func (p *Plugin) handleAccept(w http.ResponseWriter, r *http.Request) { | |
return | ||
} | ||
|
||
p.trackAcceptIssue(userID) | ||
|
||
userName := p.listManager.GetUserName(userID) | ||
|
||
message := fmt.Sprintf("@%s accepted a Todo you sent: %s", userName, todoMessage) | ||
|
@@ -303,6 +358,7 @@ func (p *Plugin) handleComplete(w http.ResponseWriter, r *http.Request) { | |
p.handleErrorWithCode(w, http.StatusInternalServerError, "Unable to complete issue", err) | ||
return | ||
} | ||
p.trackCompleteIssue(userID) | ||
|
||
userName := p.listManager.GetUserName(userID) | ||
replyMessage := fmt.Sprintf("@%s completed a todo attached to this thread", userName) | ||
|
@@ -344,6 +400,8 @@ func (p *Plugin) handleRemove(w http.ResponseWriter, r *http.Request) { | |
return | ||
} | ||
|
||
p.trackRemoveIssue(userID) | ||
|
||
userName := p.listManager.GetUserName(userID) | ||
replyMessage := fmt.Sprintf("@%s removed a todo attached to this thread", userName) | ||
p.postReplyIfNeeded(issue.PostID, replyMessage, issue.Message) | ||
|
@@ -388,6 +446,8 @@ func (p *Plugin) handleBump(w http.ResponseWriter, r *http.Request) { | |
return | ||
} | ||
|
||
p.trackBumpIssue(userID) | ||
|
||
if foreignUser == "" { | ||
return | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package main | ||
|
||
type telemetrySource string | ||
|
||
const ( | ||
sourceCommand telemetrySource = "command" | ||
sourceWebapp telemetrySource = "webapp" | ||
) | ||
|
||
func (p *Plugin) trackCommand(userID, command string) { | ||
p.tracker.TrackUserEvent("command", userID, map[string]interface{}{ | ||
"command": command, | ||
}) | ||
} | ||
|
||
func (p *Plugin) trackAddIssue(userID string, source telemetrySource, attached bool) { | ||
p.tracker.TrackUserEvent("add_issue", userID, map[string]interface{}{ | ||
"source": source, | ||
"attached": attached, | ||
}) | ||
} | ||
|
||
func (p *Plugin) trackSendIssue(userID string, source telemetrySource, attached bool) { | ||
p.tracker.TrackUserEvent("send_issue", userID, map[string]interface{}{ | ||
"source": source, | ||
"attached": attached, | ||
}) | ||
} | ||
|
||
func (p *Plugin) trackCompleteIssue(userID string) { | ||
p.tracker.TrackUserEvent("complete_issue", userID, map[string]interface{}{}) | ||
} | ||
|
||
func (p *Plugin) trackRemoveIssue(userID string) { | ||
p.tracker.TrackUserEvent("remove_issue", userID, map[string]interface{}{}) | ||
} | ||
|
||
func (p *Plugin) trackAcceptIssue(userID string) { | ||
p.tracker.TrackUserEvent("accept_issue", userID, map[string]interface{}{}) | ||
} | ||
|
||
func (p *Plugin) trackBumpIssue(userID string) { | ||
p.tracker.TrackUserEvent("bump_issue", userID, map[string]interface{}{}) | ||
} | ||
|
||
func (p *Plugin) trackFrontend(userID, event string, properties map[string]interface{}) { | ||
if properties == nil { | ||
properties = map[string]interface{}{} | ||
} | ||
hanzei marked this conversation as resolved.
Show resolved
Hide resolved
|
||
p.tracker.TrackUserEvent("frontend_"+event, userID, properties) | ||
} | ||
|
||
func (p *Plugin) trackDailySummary(userID string) { | ||
p.tracker.TrackUserEvent("daily_summary_sent", userID, map[string]interface{}{}) | ||
} |
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.