Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial usage of share channels api (Only triggering metrics) #467
Initial usage of share channels api (Only triggering metrics) #467
Changes from 12 commits
faee759278a073ee2d0fd4f236d69a954aedd644191a5c7ad60986dc9401a4824f2b0e746269c8901f785f62edc39c7263bcf64ceb952819ca101693a999fb6132295e4314728175798cd70ab2c7e59fbd4a46cdf0d66ef5da1e6f06e85cf58956fa8ca3c14053efe718444b3a456a99f40b8ccf2ebfeb98175a5c4b0d13066ea3347cd17eFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'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?
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.
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.
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.
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.
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 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.
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.
Should this be conditional on the configuration setting?
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.
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.
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 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).
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.
Given we're not relying on this shared channels feature yet, I'm wondering if we shouldn't be destructive here and deleting the subscription?
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.
This is not "deleting the subscription", this is "undoing the channel linking done before in case of error"
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.
If shared channels was a subsystem we were relying on, I agree this code makes sense (though I'd suggest we invert and start sharing the channel first). But for the purpose of this PR -- an experiment to measure the overhead of shared channels -- we're directly tying the functionality of that core to the enduser's experience, breaking channel linking if shared channels doesn't work for some reason. Should we narrow the scope of this PR to "just an experiment" with 0 expected impact on the enduser?
cc @jespino / @wiggin77 as I sense this is a broader question for what we want to achieve here.
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.
Should this be conditional on the configuration setting?
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 don't think we need to make it conditional.
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.
See above for probably doing nothing in this case, but even if this was "live", I'm wondering why we would try to rollback the subscription here? Presumably we should still delete the subscription?
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.
This is because it is consider that the whole operation has fail, I think this is the correct behavior. You are trying to "unshare" the channel, and because you were unable, you share it again.
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'm not sure I'd expect those semantics as an enduser. I agree that in a purely transactional sense, this is ideal -- but the inability to stop sharing a channel in a subsystem internal to us shouldn't preclude disconnecting the MS Teams side. That succeeded and severs the link, even if we have to fix issue internally.
(In other words, I'd rather we harden the code to deal with shared channels that have no matching subscription, than force this error onto the enduser and prevent them from unlinking in the first place.)