Triggers for calls that exceed the limit

[dlafreniere]

Now that the limit is verified before incrementing, the triggers are still called. I'm not sure if this is the desired behaviour.

example: Limit of 5, ValueTrigger of 3

• Storage = 0/5
• tryCall(6) = false
• Storage = 0/5
• Trigger executed
• tryCall(3) = true
• Storage = 2/5
• Trigger NOT executed

[dlafreniere]

@Sytten @GuiSim

[Sytten]

If I remember correctly:

• This was the correct behavior we decided to go for at the time, because the need for my team wasn't small and precise limits like in the example and we wanted to trigger even if the user "wanted" to cross the limit.
• I fixed the "bug" were the trigger would be called multiple times at each tryCall that exceeds the limit. This is why it's not called a second time in the same bucket in your example. This is to avoid a situation were the the storage would at 4/5 and the user keeps calling tryCall(2) (generating a trigger each time).

I see that in your case this is not the desired behavior. One thing would be to check if the previous value of the limit was 0 (I believe you have that info in the triggers) and not trigger in that case. Another idea would be to set an optional "margin" from the limit for the triggers to trigger (ex: you have a limit at 200 and a margin of 10. If the user is at 150 and tryCall(60) it won't trigger, but if he is at 195 and tryCall(15) it will).

[GuiSim]

I vote against the margin mechanism. Keep it simple, this is just throttling ;)

I don't use this library right now (might in the future) but I think that you guys, as the main users, know best what the expectation is. What do developers expect when they set a limit and a trigger? When should they expect the trigger to be fired? How should this situation be handled outside Coveo, will your decision still make sense?

It's been a while since I played in Spillway. I don't think I can have a more valuable input, sorry.

[dlafreniere]

Thanks for your input guys. I agree that it should be kept simple. Given that we are not using the triggers mechanism right now, I will not touch the current behaviour, but leave this discussion around if and when somebody else who is using the feature has an opinion

[dreisch-coveo]

I'm all in favor of removing the Triggers if they're not being used by anyone.

[Sytten]

I would not removed it, I thinks its kind of important if you want to reach outside coveo. You can do nice stuff with it like I did in a hackathon: https://github.com/frederik-labbe/throttling-notifications

[malaporte]

Not sure I agree. The calling application already knows that a call is being throttled, and thus has all the liberty it wants to to whatever a trigger would, and with more context available to boot.

Unless I'm missing something, I don't see a use case that requires triggers. Considering this, I'd err on the side of simplicity here and focus only on what the lib must do. Aka I'm siding with @dreisch-coveo :grin:

[Sytten]

Hum I don't agree on that. Maybe the application knows, but doing actions in the IF statement that checks the tryCall is just bad IMO. Triggers are useful to send emails to clients or take further actions against a bad behavior. I would argue that one could keep his code much cleaner that way, but your call.

[malaporte]

I'm very much open to argumentation y'know :)

Can you detail a bit more how handling throttling at the call level is bad for you?

For my part, I'm worried that a trigger might be missing some contextual information that is readily available in the calling code to properly handle the throttling event.

[dreisch-coveo]

I agree with @Sytten that the triggers are cleaner and I'm sure that either the lib or the application can provide sufficient context most of the time -- or at most the library could allow the application to supply extra context.

For me its more a question of whether the use case is common and important enough for us to support and maintain it in the lib, vs forcing users to do something themselves.

Overall, considering the critical nature of the lib I'd agree with @malaporte that simplicity might be worth more here.

[Sytten]

A couple of other points to consider:

• Its a breaking change to remove them so that means a V3
• They are already implemented and working (last time I checked)
• Letting the user manage them might not result in very clean code At the end of the day it really depends on what you want to do with the lib. If you look at the other issues from @GuiSim or me, most of them are suggestions to add sugar around the lib to make it easier to use (like a spring filter or an annotation). But if you want to make barebone and simple it's fine too, just inform the users of that :)