[MM-48342] Stateful timers

[hanzei]

Summary

This is a first draft of state full timer. Apps can now create timers by defining a time when the timer should be executed.

The API is:

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"`
}

Ticket Link

https://mattermost.atlassian.net/browse/MM-48342

[hanzei]

@levb @javaguirre The details are still WIP, but I would appreciate a high level review of the API.

[levb]

@hanzei would you please publish your mattermost-plugin-api branch? I did not find it.

[levb]

👍

[javaguirre]

It would be nice if you add a code example in a comment of how we plan to use this to better understand everything. It's nice also for the future documentation. Thank you! 👍

[hanzei]

@javaguirre The test app contains an example usage of timers.

[levb]

@hanzei 0/5 and optional: would be nice to add a REST API test for this, at least for the happy path? If you need help figuring the tests out, I'd be happy to.

[javaguirre]

I've made some sweeps on the code, but I need to spend more time on it, just wondering about your idea to change how setCaller works.

[javaguirre]

Can't we have a dummy function in the NewService set to something 'default' as a Caller, so we could set it up later when needed?

[hanzei]

What would that default Caller look like?

[javaguirre]

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.

[javaguirre]

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
}

[codecov-commenter]

Codecov Report

Base: 20.78% // Head: 20.75% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (496456e) compared to base (d5d265e).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
- Coverage   20.78%   20.75%   -0.04%     
==========================================
  Files          79       80       +1     
  Lines        5445     5455      +10     
==========================================
  Hits         1132     1132              
- Misses       4181     4191      +10     
  Partials      132      132              

Impacted Files	Coverage Δ
apps/timer.go	0.00% <0.00%> (ø)
server/proxy/service.go	6.97% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[levb]

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 and KVList slow.

2/5 consider mattermost/mattermost#21653

[hanzei]

For reference: cluster.JobOnceScheduler calls KVList on Start() and every 5 minutes.

[hanzei]

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?

[hanzei]

I've talked with @levb offline. The solutions in #375 (comment) are the path forward, but require the cache store from #458.

[hanzei]

Notes from an offline discussion with @levb:

• Notes receiving a job create event communicate to the leader the event
• Leader writen to KV store
• Also store a list of keys per interval (e.g. per hour)
• Leader keeps a list of the jobs for the current interval in memory
• The leader is the only one who executes

On leader change:

• Read the current interval from KV store

[hanzei]

Let's merge this and reiterate on store once #458 is merged.