Added resolving beans by type #695

fatroom · 2019-06-17T22:40:36Z

Description

Inject tracer by bean reference instead of name

Motivation and Context

Currently tracer bean is injected by name. This leads to incompatibility issues with tracing-lightstep-spring-boot-starter > 0.1.5
Instead of relying on bean name for plugin registration, I've changed to loading by type.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.

...boot-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/RiptidePostProcessor.java

whiskeysierra · 2019-06-18T06:25:33Z

I believe we have a couple of other places where this could be useful. Basically whenever we inject by name right now. Logbook comes to mind, e.g.

fatroom · 2019-06-18T06:33:46Z

Another way that came into my mind: register factories in postProcessBeanDefinitionRegistry and afterwards configure them in postProcessBeanFactory. This seems to be the most API-friendly way of solving the problem, but will require a lot(?) of changes in initialisation

whiskeysierra · 2019-06-18T09:20:36Z

I believe we have a couple of other places where this could be useful. Basically whenever we inject by name right now. Logbook comes to mind, e.g.

And MeterRegistry.

Another way that came into my mind: register factories in postProcessBeanDefinitionRegistry and afterwards configure them in postProcessBeanFactory. This seems to be the most API-friendly way of solving the problem, but will require a lot(?) of changes in initialisation

We have two kinds of configuration. Some of them completely disable a bean and others really just configure it. So we would need to spread this across both.

fatroom · 2019-06-18T09:32:02Z

I would propose following:

introduce properties for *PluginFactories
leave creation of the factories for the postProcessBeanDefinitionRegistry so we got all the beans registered during appropriate phases
setup the properties from the postProcessBeanFactory so we got type resolving available for us.

If this sounds fine for you I'll start working on implementation.

We have two kinds of configuration. Some of them completely disable a bean and others really just configure it. So we would need to spread this across both.

I don't get this point. What do you mean by two kinds of configuration? Any sample in the code?

whiskeysierra · 2019-06-18T09:45:33Z

I don't get this point. What do you mean by two kinds of configuration? Any sample in the code?

riptide.clients.example:
  tracing:
    enabled: true
    tags:
      peer.service: example-service

The enabled property dictates that the bean should be registered (or not, if set to false) while the rest (e.g. tags) is meant to configure the bean (in this case OpenTracingPlugin).

So the configuration affects both the registration and wiring of beans as well as the configuration of the properties of said beans.

whiskeysierra · 2019-06-18T09:46:05Z

If this sounds fine for you I'll start working on implementation.

Looking forward to some code to discuss this further! 🎉

This reverts commit afd2b18.

fatroom · 2019-06-19T08:28:59Z

While implementing a lot of factories, I just recognised that we don't actually require that. So there's a lot more simplified approach. I haven't changed yet the metricsRegistry yet, since it leads to broken tests, and I want to figure out reasons for this behaviour.

whiskeysierra · 2019-06-19T08:34:33Z

...-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/DefaultRiptideConfigurer.java

+        bd.getConstructorArgumentValues()
+          .getIndexedArgumentValues()
+          .values().stream()
+          .filter(holder -> arg.equals(holder.getValue()))


That feels risky.

yep. There's two options:

rely on position index.

Use a cryptic constant for default value similar to https://github.com/spring-projects/spring-framework/blob/3a0f309e2c9fdbbf7fb2d348be861528177f8555/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/ValueConstants.java#L34

fatroom · 2019-06-19T15:33:51Z

So with moving away from fixed name strategy, appropriate bean resolving will happen based on primary annotation if available, or the last one in the found beans.
Still I believe this approach is better than relying on the "bean name never change" strategy, as we know that this is not the case.

...-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/DefaultRiptideConfigurer.java

whiskeysierra · 2019-07-05T12:20:33Z

...boot-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/RiptidePostProcessor.java

@@ -40,7 +40,8 @@ public void postProcessBeanDefinitionRegistry(final BeanDefinitionRegistry regis

    @Override
    public void postProcessBeanFactory(final ConfigurableListableBeanFactory beanFactory) throws BeansException {
-        // nothing to do
+        final DefaultRiptideConfigurer configurer = new DefaultRiptideConfigurer(beanFactory, Defaulting.withDefaults(properties));


Defaulting.withDefaults(properties) can be done once in setEnvironment

...pring-boot-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/ValueConstants.java

whiskeysierra · 2019-07-05T13:02:51Z

👍

fatroom · 2019-07-05T13:06:42Z

@whiskeysierra while I was describing the changes in migration guide, something struck in my mind. Afaik if multiple beans of the same type present in spring context, but no primary explicitly defined I think spring will fail the context init. But last time I saw this behaviour was somewhere during initial release of spring 4, and not sure if it's current one. Should we follow this behaviour as well here, instead of injection of the first random bean?

whiskeysierra · 2019-07-05T13:08:07Z

I believe Spring tries to fallback to injecting by name. If that fails is usually complains about non-unique beans. Ideally we would hand over all of this work to Spring.

fatroom · 2019-07-05T15:03:28Z

Switched to default spring behaviour: try to find primary bean, if not found use resolve by name.

whiskeysierra · 2019-07-05T16:59:47Z

You're missing some code coverage there.

whiskeysierra · 2019-07-16T06:43:14Z

👍

fatroom requested review from AlexanderYastrebov, lukasniemeier-zalando, patandrick and whiskeysierra as code owners June 17, 2019 22:40

Added resolving beans by type

afd2b18

zalando deleted a comment Jun 17, 2019

whiskeysierra reviewed Jun 18, 2019

View reviewed changes

...boot-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/RiptidePostProcessor.java Show resolved Hide resolved

Revert "Added resolving beans by type"

cd6beca

This reverts commit afd2b18.

Added bean post processor for injecting tracing and logbook

71ba5f6

zalando deleted a comment Jun 19, 2019

whiskeysierra reviewed Jun 19, 2019

View reviewed changes

Added resolving of meter registry by type

49d7ebb

Introduced bean reference constants

0ba0ee4

whiskeysierra reviewed Jun 21, 2019

View reviewed changes

...-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/DefaultRiptideConfigurer.java Outdated Show resolved Hide resolved

whiskeysierra reviewed Jun 21, 2019

View reviewed changes

...-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/DefaultRiptideConfigurer.java Show resolved Hide resolved

whiskeysierra reviewed Jun 21, 2019

View reviewed changes

...-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/DefaultRiptideConfigurer.java Outdated Show resolved Hide resolved

fatroom added 2 commits June 21, 2019 10:19

Fails on missing dependency beans during plugins configuration

1fd0c72

Merge branch 'master' into type_reference_resolving

94bcb47

whiskeysierra reviewed Jul 5, 2019

View reviewed changes

...pring-boot-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/ValueConstants.java Show resolved Hide resolved

Addresed PR comments

ff35e4a

fatroom added 2 commits July 5, 2019 14:49

Replaced magic annotation with hack

7ad9b9a

Added note on changed behaviour in Migration guide

31dbbb8

Updated lookup logic to be compliant with spring behaviour

d87cb62

fatroom added 2 commits July 15, 2019 19:25

Added tests for the DefaultRiptideConfigurer

f6f68d1

Inline method that was used once

7d4a1b2

whiskeysierra merged commit e04558e into zalando:master Jul 16, 2019

fatroom mentioned this pull request Jul 16, 2019

Document tracer bean requirements #715

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added resolving beans by type #695

Added resolving beans by type #695

fatroom commented Jun 17, 2019 •

edited

whiskeysierra commented Jun 18, 2019

fatroom commented Jun 18, 2019

whiskeysierra commented Jun 18, 2019

fatroom commented Jun 18, 2019

whiskeysierra commented Jun 18, 2019

whiskeysierra commented Jun 18, 2019

fatroom commented Jun 19, 2019

whiskeysierra Jun 19, 2019

fatroom Jun 19, 2019

fatroom commented Jun 19, 2019

whiskeysierra Jul 5, 2019

whiskeysierra commented Jul 5, 2019

fatroom commented Jul 5, 2019

whiskeysierra commented Jul 5, 2019

fatroom commented Jul 5, 2019

whiskeysierra commented Jul 5, 2019

whiskeysierra commented Jul 16, 2019

Added resolving beans by type #695

Added resolving beans by type #695

Conversation

fatroom commented Jun 17, 2019 • edited

Description

Motivation and Context

Types of changes

Checklist:

whiskeysierra commented Jun 18, 2019

fatroom commented Jun 18, 2019

whiskeysierra commented Jun 18, 2019

fatroom commented Jun 18, 2019

whiskeysierra commented Jun 18, 2019

whiskeysierra commented Jun 18, 2019

fatroom commented Jun 19, 2019

whiskeysierra Jun 19, 2019

Choose a reason for hiding this comment

fatroom Jun 19, 2019

Choose a reason for hiding this comment

fatroom commented Jun 19, 2019

whiskeysierra Jul 5, 2019

Choose a reason for hiding this comment

whiskeysierra commented Jul 5, 2019

fatroom commented Jul 5, 2019

whiskeysierra commented Jul 5, 2019

fatroom commented Jul 5, 2019

whiskeysierra commented Jul 5, 2019

whiskeysierra commented Jul 16, 2019

fatroom commented Jun 17, 2019 •

edited