search

betfair / cougar

Cougar is a framework for making building network exposed service interfaces easy.
http://betfair.github.io/cougar
Apache License 2.0
27 stars 18 forks source link

Implement Zipkin Support #74 #98

Closed HugoMFernandes closed 9 years ago

HugoMFernandes commented 9 years ago

https://github.com/betfair/cougar/issues/74

eswdd commented 9 years ago

Hi Hugo,

Just acknowledging the pull request. WIll probably take a while to review and merge, will let you know if I have any questions or comments.

Simon

HugoMFernandes commented 9 years ago

Cheers Simon, please do.

eswdd commented 9 years ago
richardqd commented 9 years ago

Hi All,

Hugo, looks super so far, I haven't done a deep review as yet, but echo Simon's comments!

What's your thinking on the client dependency? If it is not something you feel strongly about, I'd prefer it not to have that - the majority of Cougar services are servers only, and include no client service bindings, and so this could possibly be just another pom change for people, though hardly a major issue, it is nice to minimise them.

Thanks, Richard.

On 30 March 2015 at 22:37, Simon Matic Langford notifications@github.com wrote:

  • Mostly very happy, just a few niggles, generally in areas which are not well documented extension points within Cougar. I haven’t reviewed the unit tests beyond a brief glance - have assumed you know the subject area best.
  • Would like to see interface/method javadoc in all interfaces and class level javadoc on all classes as a minimum. Appreciate the rest of the codebase is not there, but we’re trying to improve with new submissions.
  • What’s the code coverage of your tests? Want min 80%, so if less then please increase.
  • TODOs should be converted to github issues if desirable, otherwise remove the TODO keyword.
  • Use of zipkin will currently force inclusion of cougar-client. Currently there’s no mandate for service-only cougar’s to include the client library. I can’t decide if I’m happy with this. @richardqd https://github.com/richardqd - what’s your view?

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/pull/98#issuecomment-87839467.

eswdd commented 9 years ago

I dont think theyd need a pom change. Transitive dependencies should manage this one. There'd be extra beans registered and some exposed on jmx which would look odd though. As far as i can tell the only way to resolve would be to split the module into 3 (common/server/client) which might be good as the converse situation exists in client only scenarios. On 30 Mar 2015 23:38, "richardqd" notifications@github.com wrote:

Hi All,

Hugo, looks super so far, I haven't done a deep review as yet, but echo Simon's comments!

What's your thinking on the client dependency? If it is not something you feel strongly about, I'd prefer it not to have that - the majority of Cougar services are servers only, and include no client service bindings, and so this could possibly be just another pom change for people, though hardly a major issue, it is nice to minimise them.

Thanks, Richard.

On 30 March 2015 at 22:37, Simon Matic Langford notifications@github.com wrote:

  • Mostly very happy, just a few niggles, generally in areas which are not well documented extension points within Cougar. I haven’t reviewed the unit tests beyond a brief glance - have assumed you know the subject area best.
  • Would like to see interface/method javadoc in all interfaces and class level javadoc on all classes as a minimum. Appreciate the rest of the codebase is not there, but we’re trying to improve with new submissions.
  • What’s the code coverage of your tests? Want min 80%, so if less then please increase.
  • TODOs should be converted to github issues if desirable, otherwise remove the TODO keyword.
  • Use of zipkin will currently force inclusion of cougar-client. Currently there’s no mandate for service-only cougar’s to include the client library. I can’t decide if I’m happy with this. @richardqd https://github.com/richardqd - what’s your view?

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/pull/98#issuecomment-87839467.

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/pull/98#issuecomment-87861940.

eswdd commented 9 years ago

Actually realised there's a file missing which might also be a route to fixing.

Right now there is no cougar-module-spring.xml context file which is what cougar looks for when auto-including beans in module in the server.

We have no equivalent mechanism in the client only scenario so there would need to be an explicit include of any spring context which is what i suspect you've been doing up until now.

Suggested solution (feel free to suggest something else):

Totally seperate to this i'd like to see relevant beans exposed in jmx especially anything like brave queue lengths. On 30 Mar 2015 23:38, "richardqd" notifications@github.com wrote:

Hi All,

Hugo, looks super so far, I haven't done a deep review as yet, but echo Simon's comments!

What's your thinking on the client dependency? If it is not something you feel strongly about, I'd prefer it not to have that - the majority of Cougar services are servers only, and include no client service bindings, and so this could possibly be just another pom change for people, though hardly a major issue, it is nice to minimise them.

Thanks, Richard.

On 30 March 2015 at 22:37, Simon Matic Langford notifications@github.com wrote:

  • Mostly very happy, just a few niggles, generally in areas which are not well documented extension points within Cougar. I haven’t reviewed the unit tests beyond a brief glance - have assumed you know the subject area best.
  • Would like to see interface/method javadoc in all interfaces and class level javadoc on all classes as a minimum. Appreciate the rest of the codebase is not there, but we’re trying to improve with new submissions.
  • What’s the code coverage of your tests? Want min 80%, so if less then please increase.
  • TODOs should be converted to github issues if desirable, otherwise remove the TODO keyword.
  • Use of zipkin will currently force inclusion of cougar-client. Currently there’s no mandate for service-only cougar’s to include the client library. I can’t decide if I’m happy with this. @richardqd https://github.com/richardqd - what’s your view?

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/pull/98#issuecomment-87839467.

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/pull/98#issuecomment-87861940.

HugoMFernandes commented 9 years ago

Hey Simon/Rich,

Apologies for the delay but I was away on vacations this past week. I've just read the comments and pushed changes to most of the stuff (I'll add line notes).

Regarding the general questions:

eswdd commented 9 years ago

Don't worry about time, as you can tell I don't have loads right now anyway and there's no massive rush as this is currently the only feature actively been worked on, so we're not hard against any deadline. We'll integrate when ready and then can cut a release.

One thing not mentioned - can we also have a link to a gist of the documentation so we can integrate when we get around to the doco?

HugoMFernandes commented 9 years ago

Hey Simon,

I split the config file in server/common/client, as suggested. I haven't included the client config by default in cougar-module-spring.xml as I couldn't find a good (non-hacky) way to do it. The "best" (still terrible) way would probably be to reference the compoundEmitter through getObject() and have null checks on both emitters (adding them for emission only when the compoundEmitter exists). I think forcing inclusion of the client config file (when needed) is cleaner though (any other ideas?)

Regarding JMX: exposing relevant stuff such as queue lengths (allowing us to reconfigure them during runtime) is currently not possible. I looked at the (brave) code and a considerable refactor is required to make this possible.

Regarding gist: sure, I'll write it once we're ready to merge this.

eswdd commented 9 years ago

Hi Hugo,

Happy with that.

As for the jmx it's not to allow reconfiguration at runtime - that is undesirable - it's to be able to export current values to a system like tsdb. So read only stars please.

Cheers Simon On 21 May 2015 12:25, "HugoMFernandes" notifications@github.com wrote:

Hey Simon,

I split the config file in server/common/client, as suggested. I haven't included the client config by default in cougar-module-spring.xml as I couldn't find a good (non-hacky) way to do it. The "best" (still terrible) way would probably be to reference the compoundEmitter through getObject() and have null checks on both emitters (adding them for emission only when the compoundEmitter exists). I think forcing inclusion of the client config file (when needed) is cleaner though (any other ideas?)

Regarding JMX: exposing relevant stuff such as queue lengths (allowing us to reconfigure them during runtime) is currently not possible. I looked at the (brave) code and a considerable refactor is required to make this possible.

Regarding gist: sure, I'll write it once we're ready to merge this.

— Reply to this email directly or view it on GitHub https://github.com/betfair/cougar/pull/98#issuecomment-104234385.

HugoMFernandes commented 9 years ago

Exposed samplingLevel and queue size/remaining capacity over JMX. I had to use reflection for the queue measurements (since brave doesn't expose anything), but it only runs once (on InitializingBean::afterPropertiesSet).

eswdd commented 9 years ago

Accidentally pushed before it was ready. Backed it out and then recommitted with some tweaks.