Filters support

[andredasilvapinto]

As Cougar doesn't support web.xml Filters, it would be useful to have a replacement mechanism which would let us easily add or remove processors from the request process pipeline (a similar approach is also used in other frameworks like Twitter's Finagle).

The Pre/PostProcessor concept doesn't cover the typical Filter use case as the former is attached to the EV submission and result "pointcuts" and gives access to different structures (e.g. ExecutionContext instead of the Request object).

An architecture with Filters would allow Cougar to more adaptable and configurable, making it easier to integrate framework-wide behaviour (e.g. Tracing or QOS concerns...).

[eswdd]

This is intentional, in fact Cougar 1 had a filter concept and it was removed as it is at odds with the philosophy of cougar.

Cougar is transport agnostic, hence why the processor framework sits at the EV, as this is the common point at which all transports converge. Cougar intentionally locks down and prevents access to the request for this reason.

The 2 usecases you mention are being met without the need for filters.

Perhaps you could elaborate on specific usecases you're trying to solve that you believe require Filters and we can then discuss in context?

[andredasilvapinto]

Being tied to the EV entrance and exit, the PrePostProcessors will catch other events besides external request process. The EV is an implementation-dependent and it is used for other things like submitting cougar client requests, I was referring to the need to create a separate concept. Adding a tracing framework like Zipkin - http://twitter.github.io/zipkin/ (this is our current use case), needs a Request IN pointcut (in order to access the Request headers - trace id, span id, parent span id - once per received request) and a Request OUT pointcut (in order to propagate the headers to the services below), not an EV pointcut.

Additionally, as I said previously, the current Processors give access to different type of structures. Direct access to the Request (e.g. request.getHeader(X)) would be much easier to do than e.g. to process the headers separately, store them on the Identities chain and access that on the processor.

[richardqd]

Hi Andre,

The EV is not an implementation specific thing, it services every request regardless of its source.

A request received from a JMS service does not necessarily have the fields you mention, and this approach was deliberately taken to make the core processing engine transport agnostic.

request.getHeader(X) has no meaning for requests received on a binary transport, and we should not be adding functionality that only works if it comes in via an HTTP based transport, which is why the pre-pro/post-proc mechanisms are applied at an EV level

Cheers, Richard.

On 27 June 2014 10:30, André Pinto notifications@github.com wrote:

Being tied to the EV entrance and exit, the PrePostProcessors will catch other events besides external request process. The EV is an implementation-dependent and it is used for other things like submitting cougar client requests, I was referring to the need to create a separate concept. Adding a tracing framework like Zipkin - http://twitter.github.io/zipkin/ (this is our current use case), needs a Request IN pointcut (in order to access the Request headers - trace id, span id, parent span id - once per received request) and a Request OUT pointcut (in order to propagate the headers to the services below), not an EV pointcut.

Additionally, as I said previously, it gives access to different type of structures. Direct access to the Request (e.g. request.getHeader(X)) would be much easier to do than e.g. to process the headers separately, store them on the Identities chain and access that on the processor.

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/issues/72#issuecomment-47324655.

[andredasilvapinto]

When I said implementation-specific I was referring to it not being tied with any outside standard. It is a Cougar construction and can be used for other things other than the process of incoming requests.

If in order to grant uniformity we discard relevant features of the protocols Cougar supports, then it is a very poor "support" we are offering for these protocols.

Cougar already performs protocol specific actions. Actually it even accesses the HTTP headers, eg.:

        final HttpServletRequest request = command.getRequest();
        String uuidString = request.getHeader(uuidHeader);

in com.betfair.cougar.transport.api.protocol.http.ExecutionContextFactorycom.betfair.cougar.transport.api.protocol.http.ExecutionContextFactory (Cougar 2)

Actually the Request UUID is one great example of something that should have been a configurable filter instead of behind "hard-coded" in the request process pipeline.

And yes, that code is in a package specific for HTTP, but that's not a problem to what I'm purposing here. We can/should have the possibility to configure filters on a per-protocol-basis.

[richardqd]

I understood what you were driving at, but a fundamental concept behind cougar is that you should have zero knowledge of the origination of the request. It is precisely for this reason that the request uuid stuff is dealt with outside the execution venue.

The point is that a request and pre-proc/post-proc functionality should be processable whether it originated from JMS, an internal cougar client (similar to an EJB call), a cougar to cougar client, a binary socket call

To come all the way back to the start. Cougar's purpose is not an HTTP container / nor RPC over HTTP, but as a lightweight transport agnostic RPC container. Adding support for filters for in bound http requests would be very poor, as soon as you did this and it didn't work for bin protocol requests, you'd end up offering something that didn't work for that use case.

Imagine you had just one cougar service that had a dozen service interfaces inside it. This is a common use case, and often a call goes between those services without going via any transport. As soon as you offered something HTTP specific you've stepped away from one of the key principals because then it simply wouldn't work since there would be no Http call in the process. Every single request is always guaranteed to go via the EV and its execution pipeline, so pre/post proc will still work independently of how the call is invoked. Would you really want no traceability here? If the originating request hits one service, and that service calls another in the same cougar, then you're scuppered in terms of traceability

On 27 June 2014 11:36, André Pinto notifications@github.com wrote:

When I said implementation-specific I was referring to it not being tied with any outside standard. It is a Cougar construction and can be used for other things other than the process of incoming requests.

If in order to grant uniformity we discard relevant features of the protocols Cougar supports, then it is a very poor "support" we are offering for these protocols.

Cougar already performs protocol specific actions. Actually it even accesses the HTTP headers, eg.:

    final HttpServletRequest request = command.getRequest();
    String uuidString = request.getHeader(uuidHeader);

in com.betfair.cougar.transport.api.protocol.http.ExecutionContextFactorycom.betfair.cougar.transport.api.protocol.http.ExecutionContextFactory (Cougar 2)

Actually the Request UUID is one great example of something that should have been a configurable filter instead of behind "hard-coded" in the request process pipeline.

And yes, that code is in a package specific for HTTP, but that's not a problem to what I'm purposing here. We can/should have the possibility to configure filters on a per-protocol-basis.

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/issues/72#issuecomment-47329837.

[eswdd]

Further to what @richardqd says and in answer to your question regarding uniformity, we're not aiming to use all features of all protocols we support, we're aiming to provide relevant and consistent functionality on all supported protocols. The features of a protocol or technology are just a means to an end, it's the features of the transport as a whole which we aim to deliver.

With regard to the request uuid feature you mention, this is implemented on all protocols since it's an element on the ExecutionContext and so to meet the aim of being transport agnostic we must support this on all protocols. It's in no way more valid for the HTTP transport to support this via a filter mechanism than it is in the manner it is now. (Incidentally I would suggest that the current implementation is more explicit and obvious and so easier to maintain.)

[andredasilvapinto]

I disagree.

I'm not saying that Cougar should stop having Pre/PostInterceptors on the EV, I'm asking for adding a new configurable construction on a per protocol basis.

It's OK to have a place where you can uniformly treat all the protocols, Cougar has that covered at the EV level (although there could be made a case about whether other pointcuts would be also useful), what I think is missing is an equivalent configurable way to handle protocol specific global requirements/features.

The UUID case I referred previously could be improved with a Filter because right now it is implemented in a non-configurable way. What if I want the field to be different in JMS? What if I want to use some HTTP header to generate the UUID if it is not yet given? What if I want to parse the header in a different way? What if I want to apply that operation only to specific requests? Or have a different logic based on the type of request/operation/protocol that is being used? What if I want to run some proprietary logic on the UUID, in conjunction with other request data, to validate its integrity? the list goes on... All of this would have to be configured in Cougar's code base right now. There is no way for a Cougar user to configure the process as he/she wishes. And some concerns can not be generalized or even made public in order to be implemented in Cougar's public code base. A pluggable filtering system would be much more adequate.

There are lots of use cases which would benefit from a construction like this. With a pluggable way to apply filters, Cougar could even have protocol specific plugins, distributed apart from its main code base, that could be easily assembled together in a Cougar instance through XML, without having each of them to know about the other instance filters or configurations.

A system like that would provide more isolation, flexibility, visibility and maintainability, allowing the end user to "construct" a Cougar configuration based on the set of features that he/she needs on a per protocol or global basis. Adding or removing support for crosscuting functionalities would be a matter of adding or removing one line of XML + the filters specific configuration.

[eswdd]

You are entitled to your opinion, however the main rationale for this requirement "handling protocol specific global requirements/features" is anathema to Cougar's philosophy.

Additionally, aside from the protocol independence, one of the main reasons Cougar thrived at Betfair was that flexibility of the nature you talk about was not available, meaning services would remain consistent across a company, with configurability/extensibility restricted to well known and defined mechanisms.

The list of what-if's on the UUID can all either be met with the current functionality, or make no sense in the concept of a cohesive interface (different logic by operation).

If requirements for specific functionality (as opposed to requirements for the internal architecture) are best implemented by a filters framework then we would of course consider, but I don't see any such requirements being stated here, thus I'm tagging this item as "wontfix".

[andredasilvapinto]

OK. Thank you for the discussion.

[andredasilvapinto]

Hi Simon,

Sorry for commenting here again after closing the issue, but I got some people coming to me saying that I've not expressed my idea clearly about what I was proposing here, so just to make sure we were talking about the same thing, I would like to reiterate that what I was suggesting with this, is not simply adding support for Jetty Filters, but:

• creating a new cougar concept
• which provides configurable extension points (pointcuts)
• per protocol and
• for every protocol.

i.e.: we would have an HTTPCougarFilter interface, a BinaryCougarFilter interface, a JMSCougarFilter interface... so you could have functionality only working on a single protocol if you only implement it with say, the HTTPCougarFilter, or for every protocol if you implement it with all the filters that cougar supports (this would achieve the same result as what is done now in code). This would let you access protocol specific constructs per call (which we can't do with Pre/Processors), in a configurable way (which we can't do in code right now) while at the same time having the same support for cross platform uniformity (like we have now with features being supported on every transport protocol).

So just to be clear and give a "final end" to this discussion, was that what you understood from my early messages or where we talking about different things?

[pmvilaca]

I was reading this discussion and I understand what @andredasilvapinto is trying to explain. I think that he's suggesting that a Pre/Post Processor sometimes isn't enough and doesn't give us the flexibility to implement things like Zipkin support without changing the "core" of cougar (something like a plugin). Although, based on the discussion that we had regarding the Zipkin implementation in cougar, even with something similar to a Filter, you'll need to change the ExecutionContext to store the tracing data. Do you have any idea to solve that problem @andredasilvapinto ? Something like a Map inside the ExecutionContext to keep the objects that you need, in this case for the Zipkin data but it can be used for other plugins?

[andredasilvapinto]

Exactly. It would require a way to extend the ExecutionContext, and yes, I was thinking about something like a Map so it could be generic enough to support the Filter architecture. Nevertheless, my last comment was not intended to revive this discussion but just to clarify my position, as, according to some people here at Blip, it was not completely clear.

[richardqd]

As a discussion point, as opposed to reason for or against, changing this interface would be up there with the most disruptive thing one could do to Cougar. It is implemented / extended / wrapped in hundreds of places in the code. Many applications wrap the executionContext, and therefore they'd all be broken - this would also break a majority of service invocation tests as well - as I say, if it is the right thing to do then so be it, but it would hurt!

On 1 July 2014 17:41, André Pinto notifications@github.com wrote:

Exactly. It would require a way to extend the ExecutionContext, and yes, I was thinking about something like a Map so it could be generic enough to support the Filter architecture. Nevertheless, my last comment was not intended to revive this discussion but just to clarify my position, as, according to some people here at Blip, it was not completely clear.

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/issues/72#issuecomment-47679811.

[andredasilvapinto]

With Java 8 you can implement default methods on the interfaces. Of course that would make it incompatible with Java <8 though.

Anyway, changing the ExecutionContext would probably be required for the "ZipkinFilter" use case (it might be possible to workaround that but it would not be as elegant imo), but it is not a necessity for every Filter, i.e. the adoption of a Filter architecture (what I am asking here) is not really dependent on changing the ExecutionContext.

[eswdd]

@andredasilvapinto - Whilst you never stated the idea of having a per-protocol extension point, I'd considered it but discarded on the basis outlined previously. However, the concept of ExecutionContext extension such that new all-protocol features could be added without change to the core is compelling, although I think it would be hard to get right. I'd also be concerned that this feature would be coopted to provide protocol dependent features.

I'm wholly against adding a map to the execution context, it just ends up being a dumping ground and provides a poor interface to build solutions against. We considered this approach when we rewrote Cougar to create Cougar 2 and discarded it early on.

With regards to the Zipkin implementation, the ExecutionContext needs no change, but things like the requestuuid do need some work, as outlined in the existing issues. I do feel Cougar benefits from the work outlined in #60 and #59 and they also provide sufficient extension point to support the Zipkin integration.

It does feel to me that the core feature driving this Filters request is the Zipkin integration and it's a poor example given how I feel it's best implemented to benefit Cougar at the core.

It feels to me like the Filters concept is currently quite immature, but with some work might prove to be of benefit - similarly with some more thought it may prove to be too complex.

Wrt support for Java pre 8, I anticipate this will remain a core requirement until Java 7 reaches EOL.

[andredasilvapinto]

@eswdd thank you for replying and sorry for kind of "reopening" the discussion here with the comment above, but I felt that I had to make my position a little bit clearer considering the interpretations that I found people were having.

Unfortunately, I ended up not understanding if you still think Cougar should have no Filters support or not?

Also, please allow me to disagree when you said I never stated the idea of having a per-protocol extension point by giving quotes from my posts above:

And yes, that code is in a package specific for HTTP, but that's not a problem to what I'm purposing here. We can/should have the possibility to configure filters on a per-protocol-basis. ... I'm not saying that Cougar should stop having Pre/PostInterceptors on the EV, I'm asking for adding a new configurable construction on a per protocol basis. ... allowing the end user to "construct" a Cougar configuration based on the set of features that he/she needs on a per protocol or global basis.

Nevertheless, I recognize that starting the request with the Jetty web.xml comparison and being mainly focused on HTTP tracing as that's where I'm starting to implement the Zipkin support on Mantis and Cougar 2, might have been misleading, and that's why I made the "final" comment here.

It is also true that the use case behind this requirement was primarily Zipkin but it was not the only one. I don't know if you are aware but there is an alternative QOS implementation on Cougar 2 which is different from the one in Cougar 3. This Cougar 2 implementation doesn't use the Pre/PostProcessors pointcuts and is currently hardcoded in the AbstractCommandProcessor class:

    protected void executeCommand(final T command, final ExecutionCommand executionCommand, final ExecutionContext executionContext) {
        if (qualityOfServiceInterceptor != null) {
            qualityOfServiceInterceptor.intercept(command, createCallback(command, executionCommand, executionContext));
        } else {
            doProcess(executionCommand, executionContext);
        }
    }

My personal opinion (and considering the discussion in this thread, maybe it's really just me) is that this is not a scalable way to add new functionality and I think it would become a problem especially here in an open source project for the reasons I have said above. The request for filter support originates also as a possible solution to this problem.

For the rest, I'm pretty sure you all already know my opinions and the disagreements I've stated on the other comments. I have nothing more to add to this subject as the decision is already made and thus there is no use in going on with the debate. As I said my intention was just to make sure my suggestion was entirely clear.

Sorry again for "reopening" the thread.

[eswdd]

Andre, no need to apologise, I'd rather we discuss things than just close them down because I or anyone else says so. To make it clear the discussion is open and that the decision is not already made I have reopened this item and removed the "wontfix" label - it also isn't "accepted", as I remain sceptical as to it's viability.

Apologies, i didn't mean to suggest you hadn't stated the per-protocolness, more that I hadn't understood it too mean every protocol - however I had understood (I believe correctly) that you were talking about protocol-specific features (as opposed to protocol specific implementations of features which is where I feel this request may have some validity).

I was totally unaware of a Cougar 2 implementation of QoS since it must have happened since I left Betfair. Given how I implemented the Cougar 3 one earlier it seems a shame someone decided to implement it in this manner rather than just to backport the change I had done or just use Cougar 3, however I don't know the full circumstances surrounding the change.

One concern I think of having a flexible mechanism is the danger that peoples desires for extension will rarely be fed back to the main project and so Cougar will not really progress. However I'm willing to consider the proposal if it could be fleshed out to resolve some of the issues raised here and perhaps demonstrated how it might then have helped the 2 use cases mentioned - regardless as to how we decide to implement those features and as to whether the filters approach would have been the best.

[andredasilvapinto]

Regarding QoS I also don't know the entire reasoning behind the decision and I don't really know much about both implementations in order to identify the differences between the features, but I know that one of the reasons why it is not a backport is because the pointcut apparently had to be different. It seems that the entrance to the EV pointcut, for some reason, wasn't useful for the feature requirement and in that case there is really nothing much more elegant that could have been done than to hardcode the feature. I think the QoS question should probably be coordinated with the Foundations team as they were the ones that have implemented the feature and are the current owners and the ones developing for Cougar 2.

I don't think having the possibility to add protocol specific features is as bad as you do, but I understand the intention to bring uniformity to the major ones (like Zipkin). And you are right, I wouldn't forbid that possibility (unless we saw it was damaging Cougar), but I think a Filters system (as a concept similar to the one from Finagle that I referred on the first post) is still much valuable even if we enforce an "all-or-nothing" rule for protocol support.

I will try to sketch a possible filter-based implementation for both use cases and then post it here.

[eswdd]

Great thanks @andredasilvapinto - some suggestions on how to resolve the issues mentioned in here regarding ExecutionContext extension would also be great - by anyone..

[andredasilvapinto]

@eswdd sorry for the late reply, but I had other things going on during the first week and then I went on holidays. I've finally managed to get a sketch going on for a possible implementation of Filters in Cougar.

Here it is: https://github.com/andredasilvapinto/cougar/commit/f7db74eab8f4e182b8c31096b3d866015b69ed4b

I'm not sure how we should deal with the else branch in SocketTransportCommandProcessor (see //TODO: ExecutionContext needed for applyBeforeFilters) as we have no ExecutionContext available there... Also for the case of multiple commands on the JSON RPC calls I assumed the pointcuts to be per request and not per command (this is a case where the filter logic differs even more from the Pre/PostProcessors).

I've not implemented the Zipkin logic yet as I'm still not sure how you want to do that, but with this Filter implementation we have access to the ExecutionContextWithTokens, so we can use the Identities to store that data or change the ExecutionContext if we end up making it partially mutable. If you find it useful we can try to grab other data objects to the Filter interface.

[eswdd]

Closing as per various . context resolver in #82 is closest we will get to filters.