feat: add methods to verify service invocation #121

pierrechristinimsa · 2024-12-23T22:00:58Z

Description

Proposition of implementation for issue Add a method to MicrocksContainer class to get invocations count of a service SOAP #91
It mainly consist, at library consumer side, of two methods:
- one called "verify", that allows to simply know if a service mock has been invoked.
- another one called "getServiceInvocationsCount", that allows to know the invocations' count for a given service
README.MD have been updated to mentions these methods.
There are also other methods exposed to be able to pass the invocation day, as the Metrics REST API makes it possible. But it shouldn't be really useful. That's why I haven't mentioned them in the README.MD.
Under the hood :
- As I said before, I'm calling the Microcks Metrics REST API to obtain this data.
- I had to do a Thread.sleep(...) to avoid race condition issue with the Metrics REST API.

It still lacks of more detailed results.
For instance, for SOAP Mock: it is still impossible to know exactly the count of invocations for a given Operation of the mocked Service. But that doesn't seem to be managed by the microcks server itself, or at least not exposed by the Metrics REST API.

I would also have liked to make the sleep time possibly configurable, but didn't manage to do this simply.

Please feel free to bring feedbacks, thanks by advance.

Related issue(s)

See also #91

github-actions · 2024-12-23T22:01:10Z

👋 @pierrechristinimsa

Welcome to the Microcks community! 💖

Thanks and congrats 🎉 for opening your first pull request here! Be sure to follow the pull request template or please update it accordingly.

Hope you have a great time there!

pierrechristinimsa · 2024-12-24T14:37:14Z

Hello
It seems like I'm facing a race condition issue.

If the unit test code calls the Microcks Metrics REST API too quickly after the desired service have been invoked, randomly (about 1 time out of 2) the API responds as if the service wasn't invoked.
As a first workaround, I added a Thread.sleep(100) and I haven't encountered this race condition issue anymore, but I'm not pleased doing so.

@lbroudoux and Microcks community : have you ever heard of this kind of issue with Microcks Metrics REST API ?
Do you have an idea on how this could be handled in a more elegant and robust way ?

You will find this code, calling Thread.sleep(...) in last changes I just pushed.

lbroudoux · 2024-12-24T15:12:36Z

Hi @pierrechristinimsa
Thanks for contributing to this! I'll have a review of the PR asap - probably on Thursday.

Regarding the race condition you're facing: this is expected as the processing of invocation metrics (association to correct day/time and increment of the counter in the database) is done in an asynchronous way vs. the mock controller. I think that adding a timer cannot be avoided to fix the race condition... I just wonder if it wouldn't be better to put this directly in the library method to avoid the consumer worrying about this... 🤔

pierrechristinimsa · 2024-12-24T16:21:34Z

Hi @pierrechristinimsa Thanks for contributing to this! I'll have a review of the PR asap - probably on Thursday.

Regarding the race condition you're facing: this is expected as the processing of invocation metrics (association to correct day/time and increment of the counter in the database) is done in an asynchronous way vs. the mock controller. I think that adding a timer cannot be avoided to fix the race condition... I just wonder if it wouldn't be better to put this directly in the library method to avoid the consumer worrying about this... 🤔

Thank you for your reactive answer and explanations @lbroudoux.

I think that's what I've done : the sleep is hard-coded inside the library method, so the consumer shouldn't be aware of this.

I would have liked to make this sleep time possibly configurable, but as I'm not used to do this, I didn't manage to do this simply.

By the way, I remove the "Draft" state of this PR and I remain available to make some changes if needed.

lbroudoux · 2024-12-24T16:46:35Z

src/test/java/io/github/microcks/testcontainers/MicrocksContainerTest.java

-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;


Could you please restore the explicit import list?

lbroudoux · 2024-12-24T16:46:59Z

src/main/java/io/github/microcks/testcontainers/MicrocksContainer.java

-import java.util.List;
-import java.util.Set;
+import java.text.SimpleDateFormat;
+import java.util.*;


Could you please restore the explicit imports list?

lbroudoux · 2024-12-24T16:51:11Z

src/main/java/io/github/microcks/testcontainers/MicrocksContainer.java

+      String encodedServiceName = UrlEscapers.urlFragmentEscaper().escape(serviceName);
+      String encodedServiceVersion = UrlEscapers.urlFragmentEscaper().escape(serviceVersion);


I'm not sure this is needed as most of the HTTP client libraries or utilities are doing the encoding of the URL. Could you please check this one? If actually needed, couldn't we use instead the standard URLEncoder.encode() to avoid depending on third-party libs?

I encountered space character encoding issue.
Using URLEncoder.encode(), with UTF-8, it is changed in "%20".
Whereas the Metrics REST API expects the space character to be changed to "+".
Maybe there is another way to encode the service name so as it is correctly sent to the Metrics REST API ?

lbroudoux · 2024-12-24T16:52:34Z

src/main/java/io/github/microcks/testcontainers/MicrocksContainer.java

+      }
+
+      try {
+         Thread.sleep(100); // to avoid race condition issue when requesting Microcks Metrics REST API


👍 See https://github.com/microcks/microcks/blob/master/webapp/src/main/java/io/github/microcks/web/MockControllerCommons.java#L337 => the processing of invocation event metrics is anychronous.

Thank you for the link, interesting to see notably the event management.

lbroudoux · 2024-12-24T16:53:46Z

src/main/java/io/github/microcks/testcontainers/model/DailyInvocationStatistic.java

+/*
+ * Microcks API v1.10
+ * API offered by Microcks, the Kubernetes native tool for API and microservices mocking and testing (microcks.io)
+ *
+ * The version of the OpenAPI document: 1.10.0
+ * Contact: [email protected]
+ *
+ * NOTE: This class comes from microcks-java-client project : https://github.com/microcks/microcks-java-client
+ */


Could you place here the standard header like in https://github.com/microcks/microcks-testcontainers-java/blob/main/src/main/java/io/github/microcks/testcontainers/MicrocksContainer.java#L1-L15 ?

lbroudoux · 2024-12-24T16:56:31Z

src/main/java/io/github/microcks/testcontainers/MicrocksContainer.java

+   public boolean verify(String serviceName, String serviceVersion) {
+      return this.doVerify(serviceName, serviceVersion, null);
+   }


Could you add static version of these methods with first parameters being the microcksContainerHttpEndpoint like in https://github.com/microcks/microcks-testcontainers-java/blob/main/src/main/java/io/github/microcks/testcontainers/MicrocksContainer.java#L303 ? This help making these methods available to the microcks-quarkus DevService where we don't have access to the container object itself but just to its exposed URL. Thanks!

lbroudoux

Hey @pierrechristinimsa!
This looks very good - I just suggested a few changes to stick to coding style or governance issues. Nice job!

Signed-off-by: Pierre CHRISTIN <[email protected]>

pierrechristinimsa force-pushed the feature/verify-service-invocation branch 3 times, most recently from fb98071 to 043602c Compare December 24, 2024 14:34

pierrechristinimsa force-pushed the feature/verify-service-invocation branch from 043602c to 1fd77d0 Compare December 24, 2024 16:12

pierrechristinimsa marked this pull request as ready for review December 24, 2024 16:21

pierrechristinimsa requested a review from lbroudoux as a code owner December 24, 2024 16:21

pierrechristinimsa force-pushed the feature/verify-service-invocation branch from 1fd77d0 to 235e188 Compare December 24, 2024 16:37

lbroudoux reviewed Dec 24, 2024

View reviewed changes

lbroudoux requested changes Dec 24, 2024

View reviewed changes

lbroudoux added this to the 0.2.11 milestone Dec 24, 2024

lbroudoux added component/documentation Improvements or additions to documentation kind/feature New feature component/settings Relates to API/settings availables labels Dec 24, 2024

feat: add methods to verify service invocation

6ab5d12

Signed-off-by: Pierre CHRISTIN <[email protected]>

pierrechristinimsa force-pushed the feature/verify-service-invocation branch from 235e188 to 6ab5d12 Compare December 24, 2024 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add methods to verify service invocation #121

feat: add methods to verify service invocation #121

pierrechristinimsa commented Dec 23, 2024 •

edited

Loading

github-actions bot commented Dec 23, 2024

pierrechristinimsa commented Dec 24, 2024 •

edited

Loading

lbroudoux commented Dec 24, 2024

pierrechristinimsa commented Dec 24, 2024

lbroudoux Dec 24, 2024

pierrechristinimsa Dec 24, 2024

lbroudoux Dec 24, 2024

pierrechristinimsa Dec 24, 2024

lbroudoux Dec 24, 2024

pierrechristinimsa Dec 24, 2024 •

edited

Loading

lbroudoux Dec 24, 2024

pierrechristinimsa Dec 24, 2024

lbroudoux Dec 24, 2024

pierrechristinimsa Dec 24, 2024

lbroudoux Dec 24, 2024

pierrechristinimsa Dec 24, 2024

lbroudoux left a comment

		String encodedServiceName = UrlEscapers.urlFragmentEscaper().escape(serviceName);
		String encodedServiceVersion = UrlEscapers.urlFragmentEscaper().escape(serviceVersion);

feat: add methods to verify service invocation #121

Are you sure you want to change the base?

feat: add methods to verify service invocation #121

Conversation

pierrechristinimsa commented Dec 23, 2024 • edited Loading

Description

Related issue(s)

github-actions bot commented Dec 23, 2024

pierrechristinimsa commented Dec 24, 2024 • edited Loading

lbroudoux commented Dec 24, 2024

pierrechristinimsa commented Dec 24, 2024

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

pierrechristinimsa Dec 24, 2024 • edited Loading

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

lbroudoux left a comment

Choose a reason for hiding this comment

pierrechristinimsa commented Dec 23, 2024 •

edited

Loading

pierrechristinimsa commented Dec 24, 2024 •

edited

Loading

pierrechristinimsa Dec 24, 2024 •

edited

Loading