Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Micrometer Observations #1760

Merged
merged 31 commits into from Nov 14, 2022
Merged

Conversation

marcingrzejszczak
Copy link
Contributor

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 Outdated Show resolved Hide resolved
micrometer/pom.xml Outdated Show resolved Hide resolved

ClientInterceptor DEFAULT = new ClientInterceptor() {
@Override
public void beforeExecute(ClientInvocationContext context) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import java.util.HashMap;
import java.util.Map;

public class ClientInvocationContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

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 Outdated Show resolved Hide resolved
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no good. use enrich(ClientInterceptor) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marcingrzejszczak marked this pull request as ready for review Oct 4, 2022
@marcingrzejszczak
Copy link
Contributor Author

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
Copy link
Member

velo commented Oct 9, 2022

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

@marcingrzejszczak
Copy link
Contributor Author

Of course, be my guest

@marcingrzejszczak
Copy link
Contributor Author

marcingrzejszczak commented Oct 12, 2022

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
Copy link
Contributor Author

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

@marcingrzejszczak
Copy link
Contributor Author

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
Copy link

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

@velo
Copy link
Member

velo commented Nov 4, 2022

(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

velo and others added 6 commits Nov 4, 2022
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>
dependabot bot and others added 4 commits Nov 4, 2022
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
Copy link
Contributor Author

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

@OlgaMaciaszek
Copy link

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

@marcingrzejszczak
Copy link
Contributor Author

@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
Copy link
Member

velo commented Nov 9, 2022

@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
Copy link
Contributor Author

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

@marcingrzejszczak
Copy link
Contributor Author

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

velo
velo approved these changes Nov 14, 2022
@velo velo merged commit 72f379a into OpenFeign:master Nov 14, 2022
6 checks passed
@marcingrzejszczak
Copy link
Contributor Author

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

@velo
Copy link
Member

velo commented Nov 18, 2022

@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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants