Initial usage of share channels api (Only triggering metrics) #467

jespino · 2024-01-04T18:05:15Z

This PR adds the usage of the shared channels API in the plugin and start using the metrics for tracking how many "events" are generated from each side (hooks and shared channels).

This PR doesn't include the subscription to the direct messages based on "any connected user on it", that would be a follow up PR.

…n-api

jespino · 2024-01-23T16:57:19Z

server/plugin.go

@@ -426,6 +428,36 @@ func (p *Plugin) OnActivate() error {
 }
 }

+ remoteID, err := p.API.RegisterPluginForSharedChannels(model.RegisterPluginOpts{


It is important to notice that I'm registering everything here, and adding the hooks, and subscribing to channels, so the flag is just enabling and disabling the metric? Should we enable/disable the whole thing based on the configuration setting for monitoring?

There is already a setting to turn of shared channels, ~~in which case the registration will fail~~. IMO that is enough should we want to disable it. ~~However, that means OnActivate will fail and the plugin will not run~~. I think we need to decide if disabled shared channels is a fatal plugin error. Today, probably not a fatal error, but perhaps in the future. We could also look at the plugin enabling shared channels as needed.

Checking the code, registration will succeed regardless of whether shared channels service is enabled. If disabled you simply won't get any sync messages.

shared channels service is enabled

Ah, this is interesting @wiggin77. Can you elaborate on what's required to enable? Does enabling force the user to enable the full shared channels experience, or can we selectively enable this just for plugins?

@lieut-data Shared channels is enabled when a Pro or Enterprise license is used, and the ExperimentalSettings.EnableSharedChannels config flag is true. I will soon look at always having the service run, but restricting UI.

codecov-commenter · 2024-01-23T17:09:09Z

Codecov Report

Attention: 94 lines in your changes are missing coverage. Please review.

Comparison is base (4e3aa66) 41.55% compared to head (47cd17e) 41.07%.

Files	Patch %	Lines
server/plugin.go	0.00%	86 Missing ⚠️
server/command.go	71.42%	2 Missing and 4 partials ⚠️
server/api_nomock.go	0.00%	1 Missing ⚠️
server/message_hooks.go	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
- Coverage   41.55%   41.07%   -0.48%     
==========================================
  Files          22       22              
  Lines        6175     6281     +106     
==========================================
+ Hits         2566     2580      +14     
- Misses       3393     3481      +88     
- Partials      216      220       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

server/plugin.go

plugin.json

wiggin77

LGTM 👍 Few questions/comments.

Co-authored-by: Doug Lauder <[email protected]>

lieut-data

Thanks, @jespino! A few thoughts as we figure out how to roll this out and start measuring impact.

@wiggin77, could you also refresh me as to what metrics shared channels itself already exposes and how we might want to start monitoring that from a Grafana dashboard?

go.mod

plugin.json

lieut-data · 2024-01-30T19:32:56Z

server/command.go

@@ -221,6 +221,22 @@ func (p *Plugin) executeLinkCommand(args *model.CommandArgs, parameters []string
 return p.cmdError(args.UserId, args.ChannelId, "Error occurred while saving the subscription")
 }

+ if _, err = p.API.ShareChannel(&model.SharedChannel{


Should this be conditional on the configuration setting?

Not necesarilly, if you enable the shared channels it would work, if not it shouldn't be a problem. This way we exercise more machinery during this changes.

I going to put this behind the config flag to be able to reduce surface here while the metrics are off. But that can leave the system in a weird state where only after restart some channels are really shared with Shared Channels (not very important for now).

server/command.go

server/plugin.go

lieut-data · 2024-01-30T19:45:57Z

server/plugin.go

+ PluginID: pluginID,
+ CreatorID: botID,
+ AutoShareDMs: true,
+ AutoInvited: true,


Can you remind me what this does?

This doesn't require to go through the Invite process that shared channels require.

server/plugin.go

lieut-data · 2024-01-30T19:47:37Z

server/plugin.go

+ if !p.configuration.MetricsForSharedChannelsInfrastructure {
+ return nil
+ }
+ p.GetMetrics().ObserveMessageSharedChannelsEvent("attachment_created")


Wondering what the point of the "feature flag" here is? Feels like we should be turning it on or off depending on the feature flag, and always measuring metrics. If we can't reliably "turn it off", maybe the feature flag is itself not useful?

We are deciding to execute or not the metrics based on the config setting, but as you said, if there is not messages flowing because the shred channels features is off that maybe is not needed.

lieut-data · 2024-01-30T19:48:44Z

server/command.go

I'm wondering if we should consider skipping shared channels for channels, given our focus on DMs and GMs. There are edge cases here like what to do with existing subscriptions that we're not solving for, and I worry that a half-baked approach might be more confusing than adding support for shared channels explicitly later. Thoughts?

The amount of complexity of including shared channels for channels is minimal, I don't see much value on that for this specific case and for now. Let's see how it behaves, and as soon as we see a rock in the road we can disable channels, but for now, I don't feel the need.

So to be clear, we can't really disable shared channels with this PR -- we can disable collecting metrics, and we can disable the shared channels subsystem, but all the other side effects (e.g. failure to unshare a channel above) aren't feature flagged and can't be "rolled back" if they go wrong.

I'm advocating for not adding partial code unless we're going to spend the effort to productize it -- as it stands right now, we have no plan to figure out what to do with these older linked channels, and probably won't revisit until after June. It would be great if we could keep the total surface area of complexity as low as possible.

I think we are more or less align on allowing to share channels but disabling that feature optionally, giving us less surface exposure to the early adopters but giving the possibility to some of them to enable channels and start testing it.

…n-api

lieut-data · 2024-02-06T21:06:27Z

Heads up I'm looking into the build failures here, @amyblais, and may try to include a narrower scope of this PR into tomorrow's v1.7.0 build.

…Ms and GMs

lieut-data · 2024-02-07T16:42:16Z

@amyblais, I've patched the build issues, but am running into other things that make me think we should push this to v1.8 instead.

lieut-data · 2024-02-10T03:05:59Z

@jespino & @wiggin77, I made some updates to this pull request to focus the scope, mostly around measuring latency bewteen when we process the sync msg relative to when the post was created. I also updated the unit tests, opting to effect some of my simplifications above to make the job of testing easier.

I note that this won't "do" anything in environments where we aren't supporting linked channels, but should still be good fodder for our testing infrastructure.

lieut-data · 2024-02-10T03:07:23Z

One oddity is that I don't seem to be getting calls to OnSharedChannelsAttachmentSyncMsg -- or at least the metrics aren't capturing anything. I haven't debugged yet, but wondering if you have any suggestions @wiggin77?

wiggin77 · 2024-02-11T18:39:37Z

I don't seem to be getting calls to OnSharedChannelsAttachmentSyncMsg

I just double checked my test plugin that logs all the hooks calls and it is getting called. Perhaps metrics are not hooked up there.

wiggin77

LGTM 👍

jespino added 12 commits January 4, 2024 19:04

Initial usage of share channels api

faee759

WIP

278a073

Merge remote-tracking branch 'origin/main' into shared-channels-plugi…

ee2d0fd

…n-api

Share/Unshare channel on link/unlink

4f236d6

Merge remote-tracking branch 'origin/main' into shared-channels-plugi…

9a954ae

…n-api

Converting this PR into shared channels metrics

dd64419

Removing replace from go.mod

1a5c7ad

Updating the plugin API dependency in go.mod

60986dc

Generating mocks

9401a48

Fixing linter errors

24f2b0e

go mod tidy

746269c

Updating unit tests

8901f78

jespino requested review from lieut-data and wiggin77 January 23, 2024 16:54

jespino changed the title ~~Initial usage of share channels api~~ Initial usage of share channels api (Only triggering metrics) Jan 23, 2024

jespino commented Jan 23, 2024

View reviewed changes

jespino marked this pull request as ready for review January 23, 2024 16:58

wiggin77 reviewed Jan 24, 2024

View reviewed changes

server/plugin.go Outdated Show resolved Hide resolved

wiggin77 reviewed Jan 24, 2024

View reviewed changes

server/plugin.go Outdated Show resolved Hide resolved

wiggin77 reviewed Jan 24, 2024

View reviewed changes

plugin.json Outdated Show resolved Hide resolved

wiggin77 reviewed Jan 24, 2024

View reviewed changes

plugin.json Outdated Show resolved Hide resolved

wiggin77 approved these changes Jan 24, 2024

View reviewed changes

jespino and others added 4 commits January 24, 2024 09:08

Apply suggestions from code review

5f62edc

Co-authored-by: Doug Lauder <[email protected]>

Adding attachment upload metrics

39c7263

Adding error handling on initial sharing of channels on startup

bcf64ce

Fixing CI

b952819

lieut-data added this to the v1.7.0 milestone Jan 24, 2024

lieut-data requested changes Jan 30, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into shared-channels-plugi…

ca10169

…n-api

jespino added 4 commits January 31, 2024 19:30

Removing uneeded log lines

cd70ab2

Using the official mattermost public version

c7e59fb

Putting most of the code behind a configuration check

d4a46cd

Adding the last changes needed for register/unregister shared channels

f0d66ef

jespino requested a review from lieut-data February 1, 2024 18:53

Merge remote-tracking branch 'origin/main' into shared-channels-plugi…

5da1e6f

…n-api

lieut-data added 6 commits February 7, 2024 10:33

dont unwind if share channel fails, simplifying unit tests

06e85cf

comment out ShareChannel/UnshareChannel in unit tests temporarily

58956fa

add missing mockAPI.AssertExpectations

8ca3c14

add coverage for when shared channels enabled

053efe7

remove ObserveMessageHooksEvent until we can count consistently for D…

18444b3

…Ms and GMs

remove debug log for shared channels ping

a456a99

lieut-data modified the milestones: v1.7.0, v1.8.0 Feb 7, 2024

lieut-data added 3 commits February 9, 2024 22:59

rework metrics to just track sync msg delay, enable by default

f40b8cc

add info logs around shared channes

f2ebfeb

use pluginId as display name

98175a5

lieut-data requested a review from wiggin77 February 10, 2024 03:04

lieut-data approved these changes Feb 12, 2024

View reviewed changes

wiggin77 approved these changes Feb 12, 2024

View reviewed changes

wiggin77 added 3 commits February 14, 2024 14:26

Merge branch 'main' into shared-channels-plugin-api

c4b0d13

fix merge conflict

066ea33

fix golint warnings

47cd17e

wiggin77 merged commit 0c4f8d2 into main Feb 14, 2024
7 checks passed

wiggin77 deleted the shared-channels-plugin-api branch February 14, 2024 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial usage of share channels api (Only triggering metrics) #467

Initial usage of share channels api (Only triggering metrics) #467

jespino commented Jan 4, 2024 •

edited

Loading

jespino Jan 23, 2024

wiggin77 Jan 24, 2024 •

edited

Loading

wiggin77 Jan 24, 2024

lieut-data Jan 24, 2024

wiggin77 Feb 6, 2024

codecov-commenter commented Jan 23, 2024 •

edited

Loading

wiggin77 left a comment

lieut-data left a comment

lieut-data Jan 30, 2024

jespino Jan 31, 2024

jespino Feb 1, 2024

lieut-data Jan 30, 2024

jespino Jan 31, 2024

lieut-data Jan 30, 2024

jespino Jan 31, 2024

lieut-data Jan 30, 2024

jespino Jan 31, 2024

lieut-data Jan 31, 2024

jespino Feb 1, 2024

lieut-data commented Feb 6, 2024

lieut-data commented Feb 7, 2024

lieut-data commented Feb 10, 2024

lieut-data commented Feb 10, 2024

wiggin77 commented Feb 11, 2024

wiggin77 left a comment

Initial usage of share channels api (Only triggering metrics) #467

Initial usage of share channels api (Only triggering metrics) #467

Conversation

jespino commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

wiggin77 Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 23, 2024 • edited Loading

Codecov Report

wiggin77 left a comment

Choose a reason for hiding this comment

lieut-data left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lieut-data commented Feb 6, 2024

lieut-data commented Feb 7, 2024

lieut-data commented Feb 10, 2024

lieut-data commented Feb 10, 2024

wiggin77 commented Feb 11, 2024

wiggin77 left a comment

Choose a reason for hiding this comment

jespino commented Jan 4, 2024 •

edited

Loading

wiggin77 Jan 24, 2024 •

edited

Loading

codecov-commenter commented Jan 23, 2024 •

edited

Loading