Di server #846

sbayer55 · 2022-01-07T17:10:02Z

Description

Refactor Data Prepper Server to use dependency injection

Issues Resolved

Remove circular dependency between Data Prepper and Data Prepper Server

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov-commenter · 2022-01-07T23:58:02Z

Codecov Report

Merging #846 (90d9e80) into main (40d52f6) will decrease coverage by 0.80%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##               main     #846      +/-   ##
============================================
- Coverage     92.21%   91.40%   -0.81%     
- Complexity      660      672      +12     
============================================
  Files            80       82       +2     
  Lines          1940     1955      +15     
  Branches        164      165       +1     
============================================
- Hits           1789     1787       -2     
- Misses          110      129      +19     
+ Partials         41       39       -2

Impacted Files	Coverage Δ
...n/dataprepper/parser/model/MetricRegistryType.java	`100.00% <ø> (+33.33%)`	⬆️
...mazon/dataprepper/plugin/DefaultPluginFactory.java	`97.50% <ø> (-0.07%)`	⬇️
...peline/server/CloudWatchMeterRegistryProvider.java	`72.22% <33.33%> (-8.74%)`	⬇️
...mazon/dataprepper/parser/config/MetricsConfig.java	`79.48% <73.33%> (-20.52%)`	⬇️
...ataprepper/pipeline/server/HttpServerProvider.java	`82.60% <82.60%> (ø)`
...per/parser/config/DataPrepperAppConfiguration.java	`100.00% <100.00%> (ø)`
...zon/dataprepper/parser/config/DataPrepperArgs.java	`90.32% <100.00%> (ø)`
...dataprepper/pipeline/server/DataPrepperServer.java	`100.00% <100.00%> (+10.16%)`	⬆️
...aprepper/pipeline/server/ListPipelinesHandler.java	`100.00% <100.00%> (ø)`
...pper/pipeline/server/PrometheusMetricsHandler.java	`100.00% <100.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40d52f6...90d9e80. Read the comment docs.

dlvenable · 2022-01-10T17:28:18Z

data-prepper-core/src/main/java/com/amazon/dataprepper/parser/config/MetricsConfig.java

+    @Bean
+    public Optional<MeterRegistry> prometheusMeterRegistry(
+            final DataPrepperConfiguration dataPrepperConfiguration,
+            final Optional<Authenticator> optionalAuthenticator,


Taking an Optional as an argument is not a good practice. You have no non-null guarantees here, so you should do a null check, which somewhat defeats the purpose.

I noticed this in a few places. Please change these to just take the argument in without the Optional wrapper.

Depending on your Data Prepper configuration and Pipeline an Authenticator Bean may not be created / available. By default, if a requested bean is not available Spring will abort startup throwing an exception. We have three options in how to handle a case like this:

Always create an Authenticator, then use it based on configuration.

Wrapping the dependency as an Optional - JSR-330 compliant and would require the lease amount of refactoring if we switch to another DI framework like Dagger

Use the Spring native functionality @Autowired(required = false)

Another good example to consider is when creating the CloudWatchMeterRegistry. If the CloudWatchMeterRegistry is not configured, creating an instance of it cloud cause an exception to be thrown,

https://github.com/sbayer55/data-prepper/blob/di-server/data-prepper-core/src/main/java/com/amazon/dataprepper/parser/config/MetricsConfig.java#L120

@sbayer55, why not go with option 1? That sounds like it decouples DP with a dependency framework and won't require passing optionals around.

Option 1 presents issues with classes like CloudWatchMeterRegistry that would need additional refactoring. Option 3 is a happy middle ground. We remove the need for optionals while limiting the scope of this PR.

dlvenable · 2022-01-10T17:29:00Z

...per-core/src/main/java/com/amazon/dataprepper/parser/config/DataPrepperAppConfiguration.java

@@ -55,4 +57,9 @@ public DataPrepperConfiguration dataPrepperConfiguration(
            return new DataPrepperConfiguration();
        }
    }
+
+    @Bean
+    public Optional<PluginModel> pluginModel(final DataPrepperConfiguration dataPrepperConfiguration) {


This value is the authentication. So this method should be renamed to authentication from pluginModel.

dlvenable · 2022-01-10T17:30:29Z

data-prepper-core/src/main/java/com/amazon/dataprepper/parser/config/MetricsConfig.java

    @Bean
    public CompositeMeterRegistry systemMeterRegistry(
            final List<MeterBinder> meterBinders,
-            final DataPrepperConfiguration dataPrepperConfiguration
+            final List<Optional<MeterRegistry>> optionalMeterRegistries


This List<Optional<MeterRegister>> should be just List<MeterRegistery>.

dlvenable · 2022-01-10T17:41:14Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginCreator.java

@@ -23,7 +23,7 @@
 class PluginCreator {
    private static final Logger LOG = LoggerFactory.getLogger(PluginCreator.class);

-    <T> T newPluginInstance(final Class<T> pluginClass,
+    public <T> T newPluginInstance(final Class<T> pluginClass,


This shouldn't be public. The class itself is already package protected, and keeping the method package protected is clearer.

dlvenable · 2022-01-10T17:44:40Z

data-prepper-core/src/main/java/com/amazon/dataprepper/parser/config/MetricsConfig.java

+
+            final PrometheusMetricsHandler metricsHandler = new PrometheusMetricsHandler(meterRegistry);
+
+            final List<HttpContext> contextList = new ArrayList<>(2);


This bean creation has two distinct concerns involved: 1) Creating the MeterRegistry and 2) setting up the HttpServer to support it. It would be better to have a different construct handle the second concern. Perhaps there can be different beans for "server contexts" and then the HttpServer bean can assemble them all.

dlvenable · 2022-01-10T17:45:47Z

...core/src/test/java/com/amazon/dataprepper/parser/config/DataPrepperAppConfigurationTest.java

+        final Optional<PluginModel> optional = appConfiguration.pluginModel(configuration);
+
+        assertThat(optional.isPresent(), is(false));
+        verify(configuration, times(1)).getAuthentication();


FYI, verify already works only for one time. So you don't need times(1) - it is the default behavior.

cmanning09 · 2022-01-11T12:26:35Z

data-prepper-core/src/main/java/com/amazon/dataprepper/parser/config/MetricsConfig.java

+    @Bean
+    public Optional<MeterRegistry> prometheusMeterRegistry(
+            final DataPrepperConfiguration dataPrepperConfiguration,
+            final Optional<Authenticator> optionalAuthenticator,


@sbayer55, why not go with option 1? That sounds like it decouples DP with a dependency framework and won't require passing optionals around.

data-prepper-core/src/main/java/com/amazon/dataprepper/pipeline/server/DataPrepperServer.java

cmanning09 · 2022-01-11T17:10:20Z

Please update the PR description and fill in the template.

cmanning09 · 2022-01-19T11:43:43Z

@sbayer55 , please update the PR with a description of the change and fill in the template.

dlvenable

Thanks! This looks good!

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/DefaultPluginFactory.java

Signed-off-by: Steven Bayer <[email protected]>

…ctor Signed-off-by: Steven Bayer <[email protected]>

Signed-off-by: Steven Bayer <[email protected]>

…uration Signed-off-by: Steven Bayer <[email protected]>

Signed-off-by: Steven Bayer <[email protected]>

…eation and config Signed-off-by: Steven Bayer <[email protected]>

Signed-off-by: Steven Bayer <[email protected]>

sbayer55 requested a review from a team as a code owner January 7, 2022 17:10

dlvenable requested changes Jan 10, 2022

View reviewed changes

cmanning09 suggested changes Jan 11, 2022

View reviewed changes

sbayer55 force-pushed the di-server branch from 928412a to f861c3e Compare January 18, 2022 23:52

dlvenable previously approved these changes Jan 19, 2022

View reviewed changes

cmanning09 reviewed Jan 20, 2022

View reviewed changes

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/DefaultPluginFactory.java Outdated Show resolved Hide resolved

sbayer55 added 21 commits January 21, 2022 14:55

Add Spring Core dependency

be3953c

Signed-off-by: Steven Bayer <[email protected]>

Added public constructor to DataPrepper Class

6d391b6

Signed-off-by: Steven Bayer <[email protected]>

Added DataPrepper instance now constructed by Spring DI

f637308

Signed-off-by: Steven Bayer <[email protected]>

Added DI based unit test for Data Prepper class

ea86ade

Signed-off-by: Steven Bayer <[email protected]>

Create DataPrepperConfigurate in bean provider method

492dfd4

Signed-off-by: Steven Bayer <[email protected]>

Added DataPrepper unit tests

9fae630

Signed-off-by: Steven Bayer <[email protected]>

Decoupled system metrics from DataPrepper

1d594a5

Signed-off-by: Steven Bayer <[email protected]>

Updated DP Test object scope

bce2c26

Signed-off-by: Steven Bayer <[email protected]>

DataPrepperServerTest refactored to work with new DataPrepper constru…

f4205e8

…ctor Signed-off-by: Steven Bayer <[email protected]>

Refactored DataPrepperConfigurationTests

8ce2584

Signed-off-by: Steven Bayer <[email protected]>

Renamed DataPrepperConfigurationConfiguration to DataPrepperAppConfig…

68d73b1

…uration Signed-off-by: Steven Bayer <[email protected]>

Remove debug messages

5c1ff83

Signed-off-by: Steven Bayer <[email protected]>

Added unit tests

eab5623

Signed-off-by: Steven Bayer <[email protected]>

Changed DataPrepperServer to Lazy bean

6cd7ded

Signed-off-by: Steven Bayer <[email protected]>

Fixed bug - never checked for logstash config

321d3eb

Signed-off-by: Steven Bayer <[email protected]>

Added unit tests for command line args

22be738

Signed-off-by: Steven Bayer <[email protected]>

Removed unused import

d70ab4c

Signed-off-by: Steven Bayer <[email protected]>

Renamed test from foo

2c4ad8e

Signed-off-by: Steven Bayer <[email protected]>

Added copywrite comments

5f1a131

Signed-off-by: Steven Bayer <[email protected]>

Remove commented code

6c54d84

Signed-off-by: Steven Bayer <[email protected]>

refactor test to assert against hard coded string

c5557b8

Signed-off-by: Steven Bayer <[email protected]>

sbayer55 added 21 commits January 21, 2022 14:55

added final modifier to main class variables

75e9e52

Signed-off-by: Steven Bayer <[email protected]>

Fixed misplaced Nullable

9ef1060

Signed-off-by: Steven Bayer <[email protected]>

Add Spring Core dependency

e6d8041

Signed-off-by: Steven Bayer <[email protected]>

Converted DataPrepperServer to use Spring Dependency Injection

85c84ae

Signed-off-by: Steven Bayer <[email protected]>

Added DataPrepperServer constructor test

835bee5

Signed-off-by: Steven Bayer <[email protected]>

remove unused swing dependency

71f13e8

Signed-off-by: Steven Bayer <[email protected]>

Added testing for new classes

ec61194

Signed-off-by: Steven Bayer <[email protected]>

Added Server config unit test

eebc8c7

Signed-off-by: Steven Bayer <[email protected]>

Added DataPrepperServerConfiguration unit tests

7d8f1f6

Signed-off-by: Steven Bayer <[email protected]>

Added final modifier where possible

a2ccce5

Signed-off-by: Steven Bayer <[email protected]>

Only create Cloud Watch Provider if configured

2b1ed15

Signed-off-by: Steven Bayer <[email protected]>

Changed when AWSCloudMeter are registered

4614301

Signed-off-by: Steven Bayer <[email protected]>

Removed unused import

ed97a93

Signed-off-by: Steven Bayer <[email protected]>

Removed invalid unit test

35729ff

Signed-off-by: Steven Bayer <[email protected]>

Removed unused import

70a8aa8

Signed-off-by: Steven Bayer <[email protected]>

Added unit tests on ListPipelinesHandler

73367ac

Signed-off-by: Steven Bayer <[email protected]>

Add unit tests for PrometheusMetricsHandler

e2849a4

Signed-off-by: Steven Bayer <[email protected]>

Remove Optional types from Spring configuration files

1b5aee4

Signed-off-by: Steven Bayer <[email protected]>

Remove Optional types from Spring configuration files, split meter cr…

2f116f5

…eation and config Signed-off-by: Steven Bayer <[email protected]>

Added CloudWatchMeterRegistryProvider bean factory method

b4a4c11

Signed-off-by: Steven Bayer <[email protected]>

remove unused import

6b5fc42

Signed-off-by: Steven Bayer <[email protected]>

sbayer55 dismissed dlvenable’s stale review via 6b5fc42 January 21, 2022 20:55

sbayer55 force-pushed the di-server branch from bffcd96 to 6b5fc42 Compare January 21, 2022 20:55

Removed duplicated call to withPluginSetting

90d9e80

Signed-off-by: Steven Bayer <[email protected]>

dlvenable approved these changes Jan 21, 2022

View reviewed changes

cmanning09 approved these changes Jan 21, 2022

View reviewed changes

sbayer55 merged commit ec417ad into opensearch-project:main Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Di server #846

Di server #846

sbayer55 commented Jan 7, 2022 •

edited

Loading

codecov-commenter commented Jan 7, 2022 •

edited

Loading

dlvenable Jan 10, 2022

sbayer55 Jan 10, 2022

cmanning09 Jan 11, 2022

sbayer55 Jan 18, 2022

dlvenable Jan 10, 2022

dlvenable Jan 10, 2022

dlvenable Jan 10, 2022

dlvenable Jan 10, 2022

dlvenable Jan 10, 2022

cmanning09 Jan 11, 2022

cmanning09 commented Jan 11, 2022

cmanning09 commented Jan 19, 2022

dlvenable left a comment


		final PrometheusMetricsHandler metricsHandler = new PrometheusMetricsHandler(meterRegistry);

		final List<HttpContext> contextList = new ArrayList<>(2);

Di server #846

Di server #846

Conversation

sbayer55 commented Jan 7, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Jan 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmanning09 commented Jan 11, 2022

cmanning09 commented Jan 19, 2022

dlvenable left a comment

Choose a reason for hiding this comment

sbayer55 commented Jan 7, 2022 •

edited

Loading

codecov-commenter commented Jan 7, 2022 •

edited

Loading