GH-241 Support changing metrics-level for KclMessageDrivenChannelAdapter #242

Min3953 · 2024-03-15T09:47:21Z

Linked issue: #241

artembilan

Thank you for the contribution.
Any chances that you can modify KclMessageDrivenChannelAdapterTests to verify this new option?

artembilan · 2024-03-15T13:39:26Z

...java/org/springframework/integration/aws/inbound/kinesis/KclMessageDrivenChannelAdapter.java

+    /**
+     * Specify a metrics level to emit (DETAILED; default).
+     * @param metricsLevel {@link MetricsLevel} DETAILED for emitting all metrics.
+	 * @author Minkyu Moon


Move your author name to the class level.

artembilan · 2024-03-15T13:40:18Z

...java/org/springframework/integration/aws/inbound/kinesis/KclMessageDrivenChannelAdapter.java

@@ -267,6 +271,15 @@ public void setFanOut(boolean fanOut) {
 		this.fanOut = fanOut;
 	}

+    /**
+     * Specify a metrics level to emit (DETAILED; default).


Suggested change

* Specify a metrics level to emit (DETAILED; default).

* Specify a metrics level to emit.

* Defaults to {@link MetricsLevel#DETAILED}.

artembilan · 2024-03-15T13:40:53Z

...java/org/springframework/integration/aws/inbound/kinesis/KclMessageDrivenChannelAdapter.java

@@ -267,6 +271,15 @@ public void setFanOut(boolean fanOut) {
 		this.fanOut = fanOut;
 	}

+    /**
+     * Specify a metrics level to emit (DETAILED; default).
+     * @param metricsLevel {@link MetricsLevel} DETAILED for emitting all metrics.


Suggested change

* @param metricsLevel {@link MetricsLevel} DETAILED for emitting all metrics.

* @param metricsLevel the {@link MetricsLevel} for emitting (or not) metrics into Cloud Watch.

...java/org/springframework/integration/aws/inbound/kinesis/KclMessageDrivenChannelAdapter.java

artembilan · 2024-03-15T13:41:50Z

...java/org/springframework/integration/aws/inbound/kinesis/KclMessageDrivenChannelAdapter.java

@@ -321,13 +334,18 @@ protected void doStart() {
 				.glueSchemaRegistryDeserializer(this.glueSchemaRegistryDeserializer)
 				.retrievalSpecificConfig(retrievalSpecificConfig);

+		MetricsConfig metricsConfig = this.config.metricsConfig();
+		if(this.metricsLevel != null) {


No need in this if since this.metricsLevel cannot be null.

...java/org/springframework/integration/aws/inbound/kinesis/KclMessageDrivenChannelAdapter.java

…rivenChannelAdapter

artembilan · 2024-03-15T14:41:15Z

Please, consider to modify a test.
And no need to squash commits: just add a new one - and we will a history properly.
Then on merge it is going to be squashed.
Thanks

Min3953 · 2024-03-15T14:45:42Z

Thank you for the contribution. Any chances that you can modify KclMessageDrivenChannelAdapterTests to verify this new option?

Thank you for your suggestion for my first contribution.
I have tried to write test that it is right to set MetricsLevel of MetricsConfig by that option, But it is not exposed public.
So I can't write because I didn't know what you wanted to test.
Should there not be metric in CloudWatch that is emitted from kcl consumer?

artembilan · 2024-03-15T14:55:24Z

Should there not be metric in CloudWatch that is emitted from kcl consumer?

Well, it is not this project responsibility.
I believe there is just enough to be sure that property we set for this new option is propagated properly downstream into the Scheduler.
We have a org.springframework.integration.test.util.TestUtils.getPropertyValue() utility to get the underlying property via reflection.
In our case I guess it would be like this:

TestUtils.getPropertyValue(kclMessageDrivenChannelAdapter, "scheduler.metricsFactory.metricsLevel", MetricsLevel.class)

and compare it with whatever we provide in the kclMessageDrivenChannelAdapter bean configuration.

Min3953 · 2024-03-15T15:14:31Z

Thank you for your suggestion.

artembilan

And your name to that modified test class.

artembilan · 2024-03-15T15:46:23Z

...st/java/org/springframework/integration/aws/kinesis/KclMessageDrivenChannelAdapterTests.java

@@ -130,6 +150,7 @@ public KclMessageDrivenChannelAdapter kclMessageDrivenChannelAdapter() {
 			adapter.setConverter(String::new);
 			adapter.setConsumerGroup("single_stream_group");
 			adapter.setFanOut(false);
+			adapter.setMetricsLevel(MetricsLevel.fromName("NONE"));


It is just enough to use MetricsLevel.NONE.

artembilan · 2024-03-15T15:47:00Z

...st/java/org/springframework/integration/aws/kinesis/KclMessageDrivenChannelAdapterTests.java

+			"scheduler.metricsFactory.metricsLevel",
+			MetricsLevel.class
+		);
+		MetricsLevel expectedMetricsLevel = TestUtils.getPropertyValue(


We don't need to do this. There is just enough to compare with the MetricsLevel.NONE.

artembilan

All good, but no we have a Checkstyle violations:

Error: eckstyle] [ERROR] /home/runner/work/spring-integration-aws/spring-integration-aws/src/test/java/org/springframework/integration/aws/kinesis/KclMessageDrivenChannelAdapterTests.java:24:1: 'org.springframework.integration.test.util.TestUtils' should be separated from previous imports. [ImportOrder]

Error: eckstyle] [ERROR] /home/runner/work/spring-integration-aws/spring-integration-aws/src/test/java/org/springframework/integration/aws/kinesis/KclMessageDrivenChannelAdapterTests.java:25:1: Wrong order for 'software.amazon.awssdk.core.SdkBytes' import. [ImportOrder]
> Task :checkstyleTest
Error: eckstyle] [ERROR] /home/runner/work/spring-integration-aws/spring-integration-aws/src/test/java/org/springframework/integration/aws/kinesis/KclMessageDrivenChannelAdapterTests.java:46:1: Wrong order for 'software.amazon.kinesis.metrics.MetricsLevel' import. [ImportOrder]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration-aws/spring-integration-aws/src/test/java/org/springframework/integration/aws/kinesis/KclMessageDrivenChannelAdapterTests.java:128:25: Reference to instance variable 'kclMessageDrivenChannelAdapter' needs "this.". [RequireThis]

Please, run locally gradlew check to see all of the with recommendations how to fix.

Min3953 · 2024-03-15T16:00:29Z

I'm sorry to bother you. I checked Github Actions job, and fixed failure.
I trusted IDE setting too much.

artembilan · 2024-03-15T16:07:33Z

Thank you for contribution; looking forward for more!

artembilan requested changes Mar 15, 2024

View reviewed changes

Min3953 force-pushed the main branch from 0062ec3 to 437c1e6 Compare March 15, 2024 14:26

artembilan requested changes Mar 15, 2024

View reviewed changes

...java/org/springframework/integration/aws/inbound/kinesis/KclMessageDrivenChannelAdapter.java Show resolved Hide resolved

Min3953 force-pushed the main branch from 437c1e6 to b3f3d19 Compare March 15, 2024 14:30

spring-projectsGH-241: Support changing metrics-level for KclMessageD…

f4e1fc6

…rivenChannelAdapter

Min3953 force-pushed the main branch from b3f3d19 to f4e1fc6 Compare March 15, 2024 14:39

Min3953 requested a review from artembilan March 15, 2024 14:47

Add test that metricsLevel is set correctly

a78fc6c

artembilan requested changes Mar 15, 2024

View reviewed changes

Modify test body and add author

4403ad0

Min3953 requested a review from artembilan March 15, 2024 15:51

artembilan requested changes Mar 15, 2024

View reviewed changes

문민규 added 2 commits March 16, 2024 00:55

Fix import order

88b2484

Add reference to instance variable

b38b146

artembilan merged commit cf12660 into spring-projects:main Mar 15, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-241 Support changing metrics-level for KclMessageDrivenChannelAdapter #242

GH-241 Support changing metrics-level for KclMessageDrivenChannelAdapter #242

Min3953 commented Mar 15, 2024

artembilan left a comment

artembilan Mar 15, 2024

artembilan Mar 15, 2024

artembilan Mar 15, 2024

artembilan Mar 15, 2024

artembilan commented Mar 15, 2024

Min3953 commented Mar 15, 2024 •

edited

Loading

artembilan commented Mar 15, 2024

Min3953 commented Mar 15, 2024

artembilan left a comment

artembilan Mar 15, 2024

artembilan Mar 15, 2024

artembilan left a comment

Min3953 commented Mar 15, 2024

artembilan commented Mar 15, 2024

	* Specify a metrics level to emit (DETAILED; default).
	* Specify a metrics level to emit.
	* Defaults to {@link MetricsLevel#DETAILED}.

	* @param metricsLevel {@link MetricsLevel} DETAILED for emitting all metrics.
	* @param metricsLevel the {@link MetricsLevel} for emitting (or not) metrics into Cloud Watch.

GH-241 Support changing metrics-level for KclMessageDrivenChannelAdapter #242

GH-241 Support changing metrics-level for KclMessageDrivenChannelAdapter #242

Conversation

Min3953 commented Mar 15, 2024

artembilan left a comment

Choose a reason for hiding this comment

artembilan Mar 15, 2024

Choose a reason for hiding this comment

artembilan Mar 15, 2024

Choose a reason for hiding this comment

artembilan Mar 15, 2024

Choose a reason for hiding this comment

artembilan Mar 15, 2024

Choose a reason for hiding this comment

artembilan commented Mar 15, 2024

Min3953 commented Mar 15, 2024 • edited Loading

artembilan commented Mar 15, 2024

Min3953 commented Mar 15, 2024

artembilan left a comment

Choose a reason for hiding this comment

artembilan Mar 15, 2024

Choose a reason for hiding this comment

artembilan Mar 15, 2024

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Min3953 commented Mar 15, 2024

artembilan commented Mar 15, 2024

Min3953 commented Mar 15, 2024 •

edited

Loading