[MM-57348] Support notification metrics from the Desktop App client #2998
Conversation
| // Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. | ||
| // See LICENSE.txt for license information. |
There was a problem hiding this comment.
Shouldn't the linter complain since we are removing this header?
There was a problem hiding this comment.
I turned it off for this - I don't really wanna fight with having it in the built types, it should just be in our source code.
| @@ -97,10 +78,33 @@ class NotificationManager { | |||
| this.allActiveNotifications.delete(mention.uId); | |||
| }); | |||
|
|
|||
| mention.on('failed', () => { | |||
| this.allActiveNotifications.delete(mention.uId); | |||
| return new Promise((resolve) => { | |||
There was a problem hiding this comment.
I wonder if there is any chance this promise never gets resolved. I mean... are we positive by calling mention.show either the show or the failed code is going to execute? Is there any chance they won't (or take a worrying amount of time)?
If that were the case, we could add some timeout to resolve this if it takes longer than X (30s?).
There was a problem hiding this comment.
I was thinking that, and that would indicate a clear failure, but wouldn't really add more information in terms of metrics. If we never get an ACK as a result of this Promise not returning, then we can count that as a missed notification since we can't guarantee that show() was called or finished correctly. I think the failed() callback handles most of that, but the lack of an ACK should be just as good of an indication that something went wrong, and the metrics will reflect that as part of notification health.
There was a problem hiding this comment.
I am not so much worried about the metrics, but the memory leak of promises being created and never getting resolved.
There was a problem hiding this comment.
Ahh, that's a good point. In that case I think a timeout is warranted.
There was a problem hiding this comment.
This is now a "purist nit", so feel free to ignore.
Resolve is supposed to be used when the promise ends correctly. In theory, when it fails, we should reject the promise.
Nevertheless, I was trying to look in the internet some good reason to do it one way or another, and I think in this case would only add unneeded boilerplate to it :P
There was a problem hiding this comment.
Yeah also reject commonly causes an error state which is sort of the case, but not what we're trying to achieve here.
| @@ -32,19 +32,19 @@ class NotificationManager { | |||
|
|
|||
| if (!Notification.isSupported()) { | |||
| log.error('notification not supported'); | |||
| return; | |||
| return {result: 'error', reason: 'notification_api', data: 'notification not supported'}; | |||
There was a problem hiding this comment.
Should we consider "codifying" the reasons so that other sides (e.g. web client, server) can understand them? Maybe a common enum type or something could be an option.
There was a problem hiding this comment.
I'm thinking of doing that once I have a final final decision on how to track everything. I'll probably do that in the phase where I actually start adding metrics around this stuff :P
devinbinnie
added
4: Reviews Complete
Summary
This PR enables the Desktop App to return data around whether a notification was successful or not. It relays that data back to the web clients that support it, and the web client will push that data through the websocket as needed.
Ticket Link
https://mattermost.atlassian.net/browse/MM-57348