search

eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

Optional TCKs for REST stat metrics #529

Closed Channyboy closed 4 years ago

Channyboy commented 4 years ago

Fixes #521 @jmartisk @donbourne

Currently it's really GET heavy, but it does cover enough scenarios I believe. I'll add a few more PUT/POST requests that are basically copies of the existing GET requests.

But otherwise, it should be good for a look-see.

Signed-off-by: David Chan chdavid@ca.ibm.com

kenfinnigan commented 4 years ago

If the TCK is truly optional, I think it’s better if it were added as a separate module.

Otherwise it’s possible to inadvertently include the test and think functionality is missing

jmartisk commented 4 years ago

Yeah after all I am +1 to a separate module. As this needs a JAX-RS implementation to run, Metrics implementations may also want to use a different Arquillian container to run this than the rest of the rest-tck module (for example, we use WildFly servlet-only distribution for the rest-tck module, but for running this we will need the WildFly full distribution) and the required dependencies will be different, so I think it makes sense to have a separate module.

jmartisk commented 4 years ago

Well this will be tough, there will be several things that we will need to change so that it's possible to run with SmallRye, WildFly and Quarkus.

For these two issues I prepared commits https://github.com/jmartisk/microprofile-metrics/commit/b082349ecb78a6eff6b8cb7dc953fbfb9cde818a for the context root problem https://github.com/jmartisk/microprofile-metrics/commit/a30d62291378ea7138df88c4441e722481aede56 for the floating point number comparison problem (I don't know if there is a better and nicer solution atm, I changed greaterThan(0) to not(0) for now because I don't know how to properly implement the comparison so that it works regardless of whether the implementation exports the number as an integer, or with a decimal point)

Could you apply these two fixes? With these two commits I was able to get Quarkus pass the tests.

I haven't yet been able to run this with WildFly, but at minimum I know that we will need to make the metrics path :8080/metrics configurable too, because WildFly exposes metrics on port 9990 (while application endpoints are at :8080, so they are at different ports!). And this might get somewhat tough, because restassured uses RestAssured.baseURI and RestAssured.port fields for that, so we might end up having to change the RestAssured.port field every time we need to access the metrics endpoint during the test... I will continue trying to get it to work with WildFly and see what else we need.

Channyboy commented 4 years ago

@jmartisk I've incorporated your changes.

Optional TCK is now in its own module now as well.

Channyboy commented 4 years ago

@jmartisk Hope the latest commit addresses your application port issue?

You can use: -Dtest.url=http://localhost:9990 -Dapplication.port=8080

jmartisk commented 4 years ago

Good news, I managed to get this working (passing!) with WildFly as well as Quarkus now! Thanks a lot @Channyboy, really excellent job! I didn't need the different metrics port after all, because I changed the strategy how the test is deployed into WildFly, but I think that option will be useful, probably for guys from WildFly team itself, who will most likely use a different testing setup.

A suggestion would be to add a test for async REST methods, especially this should make sure that for an asynchronous requests, the measured time will be the real time to process the async request, not just the time until the REST method itself returns. But if we feel under too much time pressure, let's go ahead without that for now.

Channyboy commented 4 years ago

@jmartisk I've added one async test

Also a few extra post tests.

jmartisk commented 4 years ago

I will merge this and release RC2