feign icon indicating copy to clipboard operation
feign copied to clipboard

Micrometer Observations

Open marcingrzejszczak opened this issue 2 years ago • 0 comments

Introduction

Since I was the one responsible for the code in Spring Cloud Sleuth responsible for integration with Feign, this time I've decided to suggest a little change in Feign so that it will allow easier code instrumentation.

In order to achieve proper tracing support, what we need to do is to

  • start a span before the request is made
  • mutate the request headers (via RequestTemplate) before the request is made
  • make the call
  • parse the response to update span information
  • close the span

With Micrometer Observation the span information is hidden since tracing is an opt-in feature. So what we need to do is the following

  • create an observation before the request is made
  • pass the mutable request to a Observation.Context object
  • start the observation (here we are beginning to measure time / we create spans)
  • make the call
  • pass the response to the Observation.Context object
  • end the observation

Suggested changes

To achieve that in the least intrusive way (that seems to work - of course I will need to test it more thoroughly if you accept the idea behind the PR) I'm suggesting to add the following bits.

  • A ClientInterceptor to work with mutable headers and easily pass objects before and after the HTTP request / response got procesed
  • A way where a Capability would allow to mutate the BaseBuilder itself so that we can automatically add e.g. ClientInterceptors when a capability is used (now we can only wrap existing objects).

ClientInterceptor

a ClientInterceptor interface. Why not use the RequestInterceptor and ResponseInterceptors ? Because RI requires an immutable Request and we can't mutate it's headers :shrug:. With ClientInterceptor you have access before Client is called (so you have still access to RequestTemplate) AND you can also fill a holder (I used a simple Map for that) that you can then read after the method execution. That way you don't need to play around with ThreadLocals or anything like that to pass state between methods.

BaseBuilder and Capabilities

There's already a MicrometerCapability that sets things up on a FeignBuilder and that's great. But what if we wanted to pre-set the builder with a ClientInterceptor? That's not possible (at least I don't know how to do it) - that's why this small change would allow to enrich the builder itself not only the builder's components.

Things to consider

  • Currently when using the capability and observations you will get both, old and new metrics. Most likely we should opt out of part of old metrics (potentially timers & samples) if ObservationRegistry is used

Related issue https://github.com/OpenFeign/feign/issues/1734

marcingrzejszczak avatar Sep 19 '22 13:09 marcingrzejszczak