Micrometer Observations #1760

marcingrzejszczak · 2022-09-19T13:25:54Z

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 🤷. 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 #1734

micrometer/pom.xml

velo · 2022-09-19T21:42:19Z

core/src/main/java/feign/ClientInterceptor.java

+
+  ClientInterceptor DEFAULT = new ClientInterceptor() {
+    @Override
+    public void beforeExecute(ClientInvocationContext context) {}


I really don't like before/after interceptors, exactly cause they can't share information and then the API need a mutable context to address it.

If you look at other Interceptors feign has, it's an around interceptor. That gives the implementing code more flexibility to share data w/o the burden of having a mutable holder.

Another weak argument I have to use an around style is that java did that for servlet filters, so, can't be that bad =)

If you really want to expose a before/after style API, I guess we can have ClientInterceptor with the around method and then a ClientInterceptorAdapter with before/after methods.

Just to be clear, I have no problems with the context parameter, my problem is with the mutable holder.

I know feign has a lot of mutable states, and if I could I would eliminate them all, but, legacy can be hard.

Absolutely that's not a problem to do it with an around mechanism. The only concern I have is that with before and after you can have a list of these. If you have around you can't really have a list of those (unless I'm missing sth)

Actually I did manage to rewrite it to use around. Check this commit

velo · 2022-09-19T21:44:03Z

core/src/main/java/feign/ClientInvocationContext.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class ClientInvocationContext {


This is something I have been thinking about for other interceptors.... having a InvocationContext that gives you access to all possible moving parts, instead of injecting each as parameters. Really nice.

marcingrzejszczak · 2022-09-26T10:07:11Z

In general if you like how things are looking like with this PR I can continue with ensuring that we don't double instrument etc. I just wanted to know if the direction in which this PR goes is acceptable

kdavisk6 · 2022-09-26T14:20:16Z

In general if you like how things are looking like with this PR I can continue with ensuring that we don't double instrument etc. I just wanted to know if the direction in which this PR goes is acceptable

I'm pleased with the progress. You have my 👍🏻

core/src/main/java/feign/AsyncFeign.java

velo · 2022-09-27T22:02:18Z

core/src/main/java/feign/Capability.java

@@ -137,4 +141,8 @@ default <C> AsyncContextSupplier<C> enrich(AsyncContextSupplier<C> asyncContextS
  default MethodInfoResolver enrich(MethodInfoResolver methodInfoResolver) {
    return methodInfoResolver;
  }
+
+  default <B extends BaseBuilder<B>> BaseBuilder<B> enrich(BaseBuilder<B> baseBuilder) {


this doesn't make sense... the whole point of Capability is to change builders.... having a method that let you change the builder only indicates that the Capability API is incomplete.

this needs to be removed and whatever field needs customization needs to be added to the API

velo · 2022-09-27T22:03:42Z

micrometer/src/main/java/feign/micrometer/MicrometerCapability.java

+  @Override
+  public <B extends BaseBuilder<B>> BaseBuilder<B> enrich(BaseBuilder<B> baseBuilder) {
+    if (!observationRegistry.isNoop()) {
+      baseBuilder.clientInterceptor(new ObservedClientInterceptor(observationRegistry));


no good. use enrich(ClientInterceptor) instead.

That doesn't make sense cause I don't want to enrich an existing ClientInterceptor - I want to want to add a ClientInterceptor whenever this capability is being used. Regardless, I've applied the suggested change and needed to change the tests accordingly (I needed to ensure that there is a ClientInterceptor.DEFAULT that is being enriched.

I'm reverting that change cause it makes no sense. We would be overriding the provided client interceptor. In this case I don't think that capabilities are the way to go. Users will need to manually add a ClientInterceptor on a Feign.Builder and that's it.

marcingrzejszczak · 2022-10-04T11:35:28Z

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

That shouldn't really be a problem cause we're just instrumenting a single place (the exact point of making a request) so worst case scenario only that would be doubled. Of course only if the MicrometerCapability is turned on AND once adds the observed client interceptor.

I guess if this PR is the way to go we need to wait for Micrometer GA ?

velo · 2022-10-09T10:11:05Z

Do you mind if I split this PR and make ONE just for the new interceptor?

marcingrzejszczak · 2022-10-10T06:43:51Z

Of course, be my guest

marcingrzejszczak · 2022-10-12T16:44:58Z

BTW Micrometer 1.10.0 GA is planned for 7.11 at the moment (https://github.com/micrometer-metrics/micrometer/milestone/177). We could then merge the new feature + it would be great if Feign would release soon after so that Spring Cloud OpenFeign could use it before the 30.11 - it's when we're doing 2022.0.0 GA release (https://github.com/spring-cloud/spring-cloud-release/milestone/115). Do you think we have a chance of going with that schedule?

marcingrzejszczak · 2022-10-20T14:10:45Z

Hey @velo , what do you think of the plan?

marcingrzejszczak · 2022-11-03T09:30:08Z

Hey @velo I see that you've released 12.0 of feign without any of these changes (this is the only PR that has been left in this repo).

I understand that you're not planning to release a version that includes the ClientInterceptor nor the micrometer changes? It's been a month and a half since my PR was opened and it's been almost a month without any reply here. We're going GA with micrometer on 7/11 and there's Spring Cloud OpenFeign in the line and we need to plan our work somehow so it would be great if you could answer what your plans are.

OlgaMaciaszek · 2022-11-03T15:33:50Z

@velo @kdavisk6 could you take a look at Marcin's proposal?

velo · 2022-11-04T01:40:32Z

(this is the only PR that has been left in this repo).

Yes, until micrometer is released this can't be merged.
Also, here are conflicts that need to be address before merging.

I pretty much do feign releases as needed, so can do another release later on the week

Bumps [json](https://github.com/douglascrockford/JSON-java) from 20220320 to 20220924. - [Release notes](https://github.com/douglascrockford/JSON-java/releases) - [Changelog](https://github.com/stleary/JSON-java/blob/master/docs/RELEASES.md) - [Commits](https://github.com/douglascrockford/JSON-java/commits) --- updated-dependencies: - dependency-name: org.json:json dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps `kotlin.version` from 1.7.10 to 1.7.20. Updates `kotlin-stdlib-jdk8` from 1.7.10 to 1.7.20 - [Release notes](https://github.com/JetBrains/kotlin/releases) - [Changelog](https://github.com/JetBrains/kotlin/blob/v1.7.20/ChangeLog.md) - [Commits](JetBrains/kotlin@v1.7.10...v1.7.20) Updates `kotlin-reflect` from 1.7.10 to 1.7.20 - [Release notes](https://github.com/JetBrains/kotlin/releases) - [Changelog](https://github.com/JetBrains/kotlin/blob/v1.7.20/ChangeLog.md) - [Commits](JetBrains/kotlin@v1.7.10...v1.7.20) Updates `kotlin-maven-plugin` from 1.7.10 to 1.7.20 --- updated-dependencies: - dependency-name: org.jetbrains.kotlin:kotlin-stdlib-jdk8 dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: org.jetbrains.kotlin:kotlin-reflect dependency-type: direct:production update-type: version-update:semver-patch - dependency-name: org.jetbrains.kotlin:kotlin-maven-plugin dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps asm from 9.3 to 9.4. --- updated-dependencies: - dependency-name: org.ow2.asm:asm dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

marcingrzejszczak · 2022-11-04T16:17:00Z

I've updated the PR to remove conflicts + I extended the feature to work with Async builder too.

OlgaMaciaszek · 2022-11-07T09:43:13Z

@velo could you please tak a look after the changes?

marcingrzejszczak · 2022-11-08T11:49:53Z

@velo if you merge this and make a release we can then upgrade our version in Spring Cloud OpenFeign. What's the schedule of Feign so that we know how to plan things in the Spring Cloud train?

velo · 2022-11-09T21:09:20Z

@velo if you merge this and make a release we can then upgrade our version in Spring Cloud OpenFeign. What's the schedule of Feign so that we know how to plan things in the Spring Cloud train?

@marcingrzejszczak I refactored your change, to eliminate the need for another interceptor... we already have two o them, and introducing a third one didn't felt right just to intercept the client.

#1828

lemme know if that cover your needs. I will still try to reduce my change scope, as 600 lines still looks a lot

marcingrzejszczak · 2022-11-10T10:41:21Z

I've reviewed the PR and that looks like it should cover all needs indeed!

…meterObservations

marcingrzejszczak · 2022-11-14T11:55:30Z

I think we're ready to merge this, I've applied all the changes

marcingrzejszczak · 2022-11-14T23:44:46Z

Thanks for doing the merge! When are you planning to make the release?

velo · 2022-11-18T21:26:23Z

@marcingrzejszczak @OlgaMaciaszek
here you go:
https://github.com/OpenFeign/feign/releases/tag/12.1

BTW, would you guys consider having the spring contract as part of feign and have spring just concern with the dependency inject and the more springy side of business?

OlgaMaciaszek · 2022-11-21T15:17:38Z

Thanks a lot, @velo . Interesting idea. We'll have our team meeting in Dec. Will be able to get back to you on it then.

This was referenced Sep 19, 2022

Add support for Micrometer's new Observation API #1734

Closed

Sets up MicrometerCapabilities spring-cloud/spring-cloud-openfeign#754

Closed

velo reviewed Sep 19, 2022

View reviewed changes

marcingrzejszczak force-pushed the micrometerObservations branch from 41e6a5b to a44b24e Compare September 20, 2022 14:26

marcingrzejszczak added 6 commits September 21, 2022 11:29

WIP on Micrometer Observations

212fdf2

Added verification that metrics are measured

5e8192c

Fixed formatting

e23a805

Fixed wrong status code method call

97b23e9

Converted to using around

9119a61

Fixed compilation issues

8832aff

marcingrzejszczak force-pushed the micrometerObservations branch from a44b24e to 8832aff Compare September 21, 2022 09:29

velo requested changes Sep 27, 2022

View reviewed changes

marcingrzejszczak marked this pull request as ready for review October 4, 2022 11:16

marcingrzejszczak force-pushed the micrometerObservations branch from 6569ce8 to 1bf05c8 Compare October 4, 2022 11:30

velo and others added 6 commits November 4, 2022 12:23

prepare release 11.10

75f5a97

[ci skip] updating versions to next development iteration 11.11-SNAPSHOT

6a213cf

Preparing for next development version

4a9d1fb

Updated to latest micrometer changes

f02d3ca

Enriches via clientInterceptors

2551952

dependabot bot and others added 4 commits November 4, 2022 12:23

Applied latest changes of Micrometer

ba413e8

Removing conflicts

734272c

marcingrzejszczak force-pushed the micrometerObservations branch from 1bf05c8 to 734272c Compare November 4, 2022 16:15

marcingrzejszczak added 2 commits November 4, 2022 17:18

Polish

0421e72

Upgraded Micrometer to 1.10.0'

91ec111

marcingrzejszczak and others added 5 commits November 9, 2022 13:13

Merge branch 'master' into micrometerObservations

f03f81d

Alternative micrometer observation using capability

0784382

Ban 'repositories'

80f2e77

Merge remote-tracking branch 'origin/master' into micrometerObservations

a2f21fa

Merge branch 'master' into micrometerObservations

f1524cb

velo and others added 4 commits November 10, 2022 23:58

Merge branch 'master' into micrometerObservations

9f7a144

Merge remote-tracking branch 'velo/micrometerObservations' into micro…

07b1201

…meterObservations

Applied my own review suggestions ;)

96abdbd

Merge branch 'master' into micrometerObservations

07d77cf

marcingrzejszczak mentioned this pull request Nov 14, 2022

Micrometer observations #1828

Closed

Polish

ee58b97

velo approved these changes Nov 14, 2022

View reviewed changes

velo merged commit 72f379a into OpenFeign:master Nov 14, 2022

jonatan-ivanov mentioned this pull request Nov 19, 2022

Micrometer Observation API Support spring-cloud/spring-cloud-openfeign#793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Micrometer Observations #1760

Micrometer Observations #1760

marcingrzejszczak commented Sep 19, 2022

velo Sep 19, 2022

velo Sep 19, 2022

marcingrzejszczak Sep 20, 2022

marcingrzejszczak Sep 20, 2022 •

edited

velo Sep 19, 2022

marcingrzejszczak commented Sep 26, 2022

kdavisk6 commented Sep 26, 2022

velo Sep 27, 2022

velo Sep 27, 2022

marcingrzejszczak Sep 28, 2022 •

edited

marcingrzejszczak Sep 28, 2022

marcingrzejszczak commented Oct 4, 2022

velo commented Oct 9, 2022

marcingrzejszczak commented Oct 10, 2022

marcingrzejszczak commented Oct 12, 2022 •

edited

marcingrzejszczak commented Oct 20, 2022

marcingrzejszczak commented Nov 3, 2022

OlgaMaciaszek commented Nov 3, 2022

velo commented Nov 4, 2022

marcingrzejszczak commented Nov 4, 2022

OlgaMaciaszek commented Nov 7, 2022

marcingrzejszczak commented Nov 8, 2022

velo commented Nov 9, 2022

marcingrzejszczak commented Nov 10, 2022

marcingrzejszczak commented Nov 14, 2022

marcingrzejszczak commented Nov 14, 2022

velo commented Nov 18, 2022

OlgaMaciaszek commented Nov 21, 2022

Navigation Menu

Micrometer Observations #1760

Micrometer Observations #1760

Conversation

marcingrzejszczak commented Sep 19, 2022

Introduction

Suggested changes

ClientInterceptor

BaseBuilder and Capabilities

Things to consider

velo Sep 19, 2022

Choose a reason for hiding this comment

velo Sep 19, 2022

Choose a reason for hiding this comment

marcingrzejszczak Sep 20, 2022

Choose a reason for hiding this comment

marcingrzejszczak Sep 20, 2022 • edited

Choose a reason for hiding this comment

velo Sep 19, 2022

Choose a reason for hiding this comment

marcingrzejszczak commented Sep 26, 2022

kdavisk6 commented Sep 26, 2022

velo Sep 27, 2022

Choose a reason for hiding this comment

velo Sep 27, 2022

Choose a reason for hiding this comment

marcingrzejszczak Sep 28, 2022 • edited

Choose a reason for hiding this comment

marcingrzejszczak Sep 28, 2022

Choose a reason for hiding this comment

marcingrzejszczak commented Oct 4, 2022

velo commented Oct 9, 2022

marcingrzejszczak commented Oct 10, 2022

marcingrzejszczak commented Oct 12, 2022 • edited

marcingrzejszczak commented Oct 20, 2022

marcingrzejszczak commented Nov 3, 2022

OlgaMaciaszek commented Nov 3, 2022

velo commented Nov 4, 2022

marcingrzejszczak commented Nov 4, 2022

OlgaMaciaszek commented Nov 7, 2022

marcingrzejszczak commented Nov 8, 2022

velo commented Nov 9, 2022

marcingrzejszczak commented Nov 10, 2022

marcingrzejszczak commented Nov 14, 2022

marcingrzejszczak commented Nov 14, 2022

velo commented Nov 18, 2022

OlgaMaciaszek commented Nov 21, 2022

marcingrzejszczak Sep 20, 2022 •

edited

marcingrzejszczak Sep 28, 2022 •

edited

marcingrzejszczak commented Oct 12, 2022 •

edited