-
Notifications
You must be signed in to change notification settings - Fork 29
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
[MM-48342] Stateful timers #375
Changes from all commits
31a1574
ea11f00
1cc9fa7
a8ce6db
8bdffba
6aa7474
4a8b7c4
a8e8c53
02392ad
5eaffba
9b8628c
7ce35a6
7cbb951
8d16f04
e3786c7
496456e
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 |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. | ||
// See License for license information. | ||
|
||
package apps | ||
|
||
import ( | ||
"time" | ||
|
||
"github.com/hashicorp/go-multierror" | ||
|
||
"github.com/mattermost/mattermost-plugin-apps/utils" | ||
) | ||
|
||
// Timer s submitted by an app to the Timer API. It determines when | ||
// the app would like to be notified, and how these notifications | ||
// should be invoked. | ||
type Timer struct { | ||
// At is the unix time in milliseconds when the timer should be executed. | ||
At int64 `json:"at"` | ||
|
||
// Call is the (one-way) call to make upon the timers execution. | ||
Call Call `json:"call"` | ||
|
||
// ChannelID is a channel ID that is used for expansion of the Call (optional). | ||
ChannelID string `json:"channel_id,omitempty"` | ||
// TeamID is a team ID that is used for expansion of the Call (optional). | ||
TeamID string `json:"team_id,omitempty"` | ||
} | ||
|
||
func (t Timer) Validate() error { | ||
var result error | ||
emptyCall := Call{} | ||
if t.Call == emptyCall { | ||
result = multierror.Append(result, utils.NewInvalidError("call must not be empty")) | ||
} | ||
|
||
if t.At <= 0 { | ||
result = multierror.Append(result, utils.NewInvalidError("at must be positive")) | ||
} | ||
|
||
if time.Until(time.UnixMilli(t.At)) < 1*time.Second { | ||
result = multierror.Append(result, utils.NewInvalidError("at most be at least 1 second in the future")) | ||
} | ||
|
||
return result | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ require ( | |
github.com/gorilla/mux v1.8.0 | ||
github.com/hashicorp/go-getter v1.6.2 | ||
github.com/hashicorp/go-multierror v1.1.1 | ||
github.com/mattermost/mattermost-plugin-api v0.1.1 | ||
github.com/mattermost/mattermost-plugin-api v0.1.2-0.20221110071900-f8b73bc6795e | ||
// mmgoget: github.com/mattermost/mattermost-server/[email protected] is replaced by -> github.com/mattermost/mattermost-server/v6@ea08d47f60 | ||
github.com/mattermost/mattermost-server/v6 v6.0.0-20230113170349-ea08d47f6051 | ||
github.com/nicksnyder/go-i18n/v2 v2.2.1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,15 @@ | |
package appservices | ||
|
||
import ( | ||
"github.com/pkg/errors" | ||
|
||
"github.com/mattermost/mattermost-plugin-api/cluster" | ||
|
||
"github.com/mattermost/mattermost-plugin-apps/apps" | ||
"github.com/mattermost/mattermost-plugin-apps/server/config" | ||
"github.com/mattermost/mattermost-plugin-apps/server/incoming" | ||
"github.com/mattermost/mattermost-plugin-apps/server/store" | ||
"github.com/mattermost/mattermost-plugin-apps/utils" | ||
) | ||
|
||
type Service interface { | ||
|
@@ -17,6 +23,10 @@ type Service interface { | |
Unsubscribe(*incoming.Request, apps.Event) error | ||
UnsubscribeApp(*incoming.Request, apps.AppID) error | ||
|
||
// Timer | ||
|
||
CreateTimer(*incoming.Request, apps.Timer) error | ||
|
||
// KV | ||
|
||
KVSet(_ *incoming.Request, prefix, id string, data []byte) (bool, error) | ||
|
@@ -33,14 +43,45 @@ type Service interface { | |
GetOAuth2User(_ *incoming.Request) ([]byte, error) | ||
} | ||
|
||
type Caller interface { | ||
InvokeCall(*incoming.Request, apps.CallRequest) (*apps.App, apps.CallResponse) | ||
NewIncomingRequest() *incoming.Request | ||
} | ||
|
||
type AppServices struct { | ||
store *store.Service | ||
store *store.Service | ||
scheduler *cluster.JobOnceScheduler | ||
caller Caller | ||
|
||
conf config.Service | ||
log utils.Logger | ||
} | ||
|
||
var _ Service = (*AppServices)(nil) | ||
|
||
func NewService(store *store.Service) *AppServices { | ||
return &AppServices{ | ||
store: store, | ||
// SetCaller must be called before calling any other methods of AppsServies. | ||
// TODO: Remove this uggly hack. | ||
func (a *AppServices) SetCaller(caller Caller) { | ||
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. Can't we have a dummy function in the 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 would that default 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 the problem is not creating a default for Caller, but AppServices doing too much, why not having different services providing their own business logic? I don't think a subscription app service would need any config to create a timer, right? I need to think more about it, but if you want we can talk so I can be more helpful. 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. Something like this is what I meant (I did it fast, so it's not accurate), let me know what you think and why/why not do this (I believe a decision was made before this PR, so I'm interested) :-) // Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved.
// See License for license information.
package appservices
import (
"github.com/pkg/errors"
"github.com/mattermost/mattermost-plugin-api/cluster"
"github.com/mattermost/mattermost-plugin-apps/apps"
"github.com/mattermost/mattermost-plugin-apps/server/config"
"github.com/mattermost/mattermost-plugin-apps/server/incoming"
"github.com/mattermost/mattermost-plugin-apps/server/store"
"github.com/mattermost/mattermost-plugin-apps/utils"
)
type AppSubscriptionService interface {
// Subscriptions
Subscribe(*incoming.Request, apps.Subscription) error
GetSubscriptions(*incoming.Request) ([]apps.Subscription, error)
Unsubscribe(*incoming.Request, apps.Event) error
UnsubscribeApp(*incoming.Request, apps.AppID) error
}
type AppSubscriptionService interface {
// Timer
CreateTimer(*incoming.Request, apps.Timer) error
}
type AppKVService interface {
// KV
KVSet(_ *incoming.Request, prefix, id string, data []byte) (bool, error)
KVGet(_ *incoming.Request, prefix, id string) ([]byte, error)
KVDelete(_ *incoming.Request, prefix, id string) error
KVList(_ *incoming.Request, namespace string, processf func(key string) error) error
KVDebugInfo(*incoming.Request) (*store.KVDebugInfo, error)
KVDebugAppInfo(*incoming.Request, apps.AppID) (*store.KVDebugAppInfo, error)
}
type AppOAuthService interface {
// Remote (3rd party) OAuth2
StoreOAuth2App(_ *incoming.Request, data []byte) error
StoreOAuth2User(_ *incoming.Request, data []byte) error
GetOAuth2User(_ *incoming.Request) ([]byte, error)
}
type Caller interface {
InvokeCall(*incoming.Request, apps.CallRequest) (*apps.App, apps.CallResponse)
NewIncomingRequest() *incoming.Request
}
type AppService struct {
store *store.Service
conf config.Service
log utils.Logger
}
type AppTimerService struct {
service *AppService
scheduler *cluster.JobOnceScheduler
caller Caller
}
type AppSubscriptionService struct {
service *AppService
} |
||
a.caller = caller | ||
} | ||
|
||
func NewService(log utils.Logger, confService config.Service, store *store.Service, scheduler *cluster.JobOnceScheduler) (*AppServices, error) { | ||
service := &AppServices{ | ||
store: store, | ||
scheduler: scheduler, | ||
conf: confService, | ||
log: log, | ||
} | ||
|
||
err := scheduler.SetCallback(service.ExecuteTimer) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to set timer callback") | ||
} | ||
|
||
err = scheduler.Start() | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to start timer scheduler") | ||
} | ||
|
||
return service, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. | ||
// See License for license information. | ||
|
||
package appservices | ||
|
||
import ( | ||
"context" | ||
"strconv" | ||
"time" | ||
|
||
"github.com/pkg/errors" | ||
|
||
"github.com/mattermost/mattermost-plugin-apps/apps" | ||
"github.com/mattermost/mattermost-plugin-apps/server/config" | ||
"github.com/mattermost/mattermost-plugin-apps/server/incoming" | ||
) | ||
|
||
type storedTimer struct { | ||
Call apps.Call `json:"call"` | ||
AppID apps.AppID `json:"app_id"` | ||
UserID string `json:"user_id"` | ||
ChannelID string `json:"channel_id,omitempty"` | ||
TeamID string `json:"team_id,omitempty"` | ||
} | ||
|
||
func (t storedTimer) Key(appID apps.AppID, at int64) string { | ||
return string(appID) + t.UserID + strconv.FormatInt(at, 10) | ||
} | ||
|
||
func (t storedTimer) Loggable() []interface{} { | ||
props := []interface{}{"user_id", t.UserID} | ||
props = append(props, "app_id", t.AppID) | ||
if t.ChannelID != "" { | ||
props = append(props, "call_team_id", t.TeamID) | ||
} | ||
if t.TeamID != "" { | ||
props = append(props, "call_channel_id", t.ChannelID) | ||
} | ||
return props | ||
} | ||
|
||
func (a *AppServices) CreateTimer(r *incoming.Request, t apps.Timer) error { | ||
err := r.Check( | ||
r.RequireActingUser, | ||
r.RequireSourceApp, | ||
t.Validate, | ||
) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
st := storedTimer{ | ||
Call: t.Call, | ||
AppID: r.SourceAppID(), | ||
UserID: r.ActingUserID(), | ||
ChannelID: t.ChannelID, | ||
TeamID: t.TeamID, | ||
} | ||
|
||
_, err = a.scheduler.ScheduleOnce(st.Key(r.SourceAppID(), t.At), time.UnixMilli(t.At), st) | ||
if err != nil { | ||
return errors.Wrap(err, "faild to schedule timer job") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (a *AppServices) ExecuteTimer(key string, props interface{}) { | ||
t, ok := props.(storedTimer) | ||
if !ok { | ||
a.log.Debugw("Timer contained unknown props. Inoring the timer.", "key", key, "props", props) | ||
return | ||
} | ||
|
||
r := a.caller.NewIncomingRequest() | ||
|
||
r.Log = r.Log.With(t) | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), config.RequestTimeout) | ||
defer cancel() | ||
r = r.WithCtx(ctx) | ||
|
||
r = r.WithDestination(t.AppID) | ||
r = r.WithActingUserID(t.UserID) | ||
|
||
context := &apps.Context{ | ||
UserAgentContext: apps.UserAgentContext{ | ||
AppID: t.AppID, | ||
TeamID: t.TeamID, | ||
ChannelID: t.ChannelID, | ||
}, | ||
} | ||
|
||
creq := apps.CallRequest{ | ||
Call: t.Call, | ||
Context: *context, | ||
} | ||
r.Log = r.Log.With(creq) | ||
_, cresp := a.caller.InvokeCall(r, creq) | ||
if cresp.Type == apps.CallResponseTypeError { | ||
if a.conf.Get().DeveloperMode { | ||
r.Log.WithError(cresp).Errorf("Timer execute failed") | ||
} | ||
return | ||
} | ||
r.Log = r.Log.With(cresp) | ||
|
||
r.Log.Debugf("Timer executed") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. | ||
// See License for license information. | ||
|
||
package httpin | ||
|
||
import ( | ||
"encoding/json" | ||
"net/http" | ||
|
||
"github.com/mattermost/mattermost-plugin-apps/apps" | ||
"github.com/mattermost/mattermost-plugin-apps/server/incoming" | ||
"github.com/mattermost/mattermost-plugin-apps/utils/httputils" | ||
) | ||
|
||
// CreateTimer create or updates a new statefull timer. | ||
// | ||
// Path: /api/v1/timer | ||
// Method: POST | ||
// Input: JSON {at, call, state} | ||
// Output: None | ||
func (s *Service) CreateTimer(r *incoming.Request, w http.ResponseWriter, req *http.Request) { | ||
var t apps.Timer | ||
|
||
err := json.NewDecoder(req.Body).Decode(&t) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
return | ||
} | ||
|
||
err = s.AppServices.CreateTimer(r, t) | ||
if err != nil { | ||
http.Error(w, err.Error(), httputils.ErrorToStatus(err)) | ||
return | ||
} | ||
} |
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.
Here's my biggest issue with this PR. JobOnceScheduler relies on
KVList
. plugin-apps stores all of the app keys in its keyspace (session and ouath2 tokens), so the list is going to be huge andKVList
slow.2/5 consider mattermost/mattermost#21653
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.
For reference:
cluster.JobOnceScheduler
callsKVList
onStart()
and every 5 minutes.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.
Could you please elaborate on how a solution that uses mattermost/mattermost#21653 would work?
I would assume only the cluster leader executes timers, correct? Once a plugin instance receives the request to create a new timer, it needs to communicate that information to the leader. How would that work?
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've talked with @levb offline. The solutions in #375 (comment) are the path forward, but require the cache store from #458.