Events for plugin communication

[enoldev]

This an implementation proposal on how can we handle plugin to plugin communication in an easier way. The concept uses "events" to which plugins can subscribe and it is implemented using the current RPC communication that mattermost-server uses to handle plugin communication.

This way, any plugin that is subscribed to an event will get the message without any other necessary action.

- Plugin API:
SubscribePluginEvent: allows to subscribe to an specific event
SendPluginEvent: allows you to send an event

- Hooks:
ReceivePluginEvent: allows you to handle events to which you have subscribed.

Example:

1. Plugin A gets subscribe to "Test" event
2. Plugin B sends a "Test" event
3. Plugin A gets the "Test" event

A simple code snippet:

Plugin A

// We can use the OnActivate hook in order to get subscribe to events
func (p *Plugin) OnActivate() {
	p.API.SubscribePluginEvent("Test")
}

// We get all events here
func (p *Plugin) ReceivePluginEvent(event string, payload interface{}) {
	
	switch event {
	case "Test":
		testEventMessage := payload.(string)
	}
}

Plugin B

func (p *Plugin) SomeMethod() {
	p.API.SendPluginEvent("Test", "test message")
}

Tests are still missing and any sort of comments on this would be appreciated.

Thanks a lot!

[enoldev]

Maybe we can set a "Work in progress" tag?

[mickmister]

@enolal826 Great job! I have a few suggestions and design questions.

@crspeller I'd like to get your thoughts on this proposal.

[mickmister]

In programs written in Go, it is common to use the variable name err when assigning a temporary error.

Suggested change

	error := api.app.servePluginEvent(event, payload, subscribedPlugin, api.manifest.Id)
	err := api.app.servePluginEvent(event, payload, subscribedPlugin, api.manifest.Id)

[mickmister]

In addition to returning the aggregate error, I think the errors should be logged individually here, as the error messages may be different per plugin.

[mickmister]

I think we should have an UnsubscribePluginEvent as well, so a plugin can dynamically unsubscribe to the event.

[mickmister]

I'm wondering if we want to make this into a defined []struct{}. We only have a use case for one field right now so it is fine as a string, as we can easily change later since the map is ephemeral.

We will want to avoid duplicate plugin ids in the subscription map. I'm thinking that a given plugin would call SubscribePluginEvent() in its OnActivate() hook method. This may cause problems if the plugin is disabled/re-enabled and is already subscribed to the event. With that in mind I think we should be removing the subscription whenever a subscribed plugin is deactivated.

[mickmister]

I think something along the lines of OnPluginEvent may be more fitting. Looking at the pattern of naming, it seems appropriate to me. A comment needs to be placed here as well. Something like

OnPluginEvent is invoked on each subscribed plugin when a PluginEvent is dispatched.

Also, I think we should define a PluginEvent struct to represent the event, with the actual data payload as an interface{} inside of the event.

[mattermod]

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

[jasonblais]

@aaronrothschild / @crspeller / @lieut-data I'm not sure if this is something that would be scheduled for review with other Toolkit related work, or related to the interplay between plugins that ChrisS is working on?

This was a November hackathon contribution from @enolal826 :)

[aaronrothschild]

@aaronrothschild / @crspeller / @lieut-data I'm not sure if this is something that would be scheduled for review with other Toolkit related work, or related to the interplay between plugins that ChrisS is working on?

This was a November hackathon contribution from @enolal826 :)

I have it on my spreadsheet for tracking, but would look to @crspeller and @lieut-data to help prioritise this work.

• Is this an architectural framework we want to build on top of?
• What are the use cases we would try to address with this approach?
• What are the shortcomings/challenges of this approach vs others we've considered?
  etc.

[lieut-data]

With the possible exception of a few naming tweaks, I think the code is fine. We actually discussed something similar a while back: https://community-release.mattermost.com/core/pl/ee9yun66ybbgzpjney5eh4a1ra, but we eventually pivoted towards directly communicating between plugins via HTTP.

@aaronrothschild, in terms of prioritization, I don't know of any specific (internal or external) plans that would require a bus like this. It may not make sense to include this in core, /but/ it might be interesting to expose this as a plugin itself -- using the existing HTTP communication, and exposing a Go package for a plugin to subscribe and publish. No changes to core required, but about twice as many hook calls.

Thoughts?

[teresa-novoa-zz]

@lieut-data @aaronrothschild where are we with this PR? Thanks!

[lieut-data]

Thanks for this effort, @enolal826! We are taking the plugin framework in a different direction right now, and so will decline these changes "as-is". I note above that I still think this could be interesting to expose as a plugin itself, requiring no server changes, but for now we won't be proceeding as-is.

(Thanks, @teresa-novoa, for the poke!)

[teresa-novoa-zz]

great, thanks @lieut-data!