Http client instrumentation TCK #3258
Conversation
shakuzen
added
module: micrometer-test
shakuzen
changed the title
| * @return name of the meter timing http client requests | ||
| */ | ||
| protected String timerName() { | ||
| return "http.client.requests"; |
There was a problem hiding this comment.
A future enhancement idea: We could inject the new naming convention mechanism here - that way if someone wants to see if they are compatible with a given standard they will be able to easily do it
| Timer timer = getRegistry().get(timerName()).tags("method", "GET", "status", "404").timer(); | ||
| // we should standardize on a value e.g. NOT_FOUND, but for backwards | ||
| // compatibility, assert is more lenient | ||
| assertThat(timer.getId().getTag("uri")).isNotEqualTo(templatedPath); |
There was a problem hiding this comment.
The Apache HTTP client instrumentation is failing this assertion now. We should fix that.
There was a problem hiding this comment.
While we should definitely test and enforce this for HTTP server instrumentation, it's not the same situation for HTTP client instrumentation. Maybe relaxing this is better.
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
| @Disabled("apache/jetty http client instrumentation currently fails this test") | ||
| void timedWhenServerIsMissing() throws IOException { | ||
| int unusedPort = 0; | ||
| try (ServerSocket server = new ServerSocket(0)) { |
There was a problem hiding this comment.
UNENCRYPTED_SERVER_SOCKET: Unencrypted server socket (instead of SSLServerSocket)
Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.
Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
|
The test coverage feels light still, but there is not so much shared behavior between our existing three HTTP clients' instrumentation. This brings up opportunities for improving the consistency and adding tests to the suite.
I'm sure there are more we can come up with. For now, I think this is a start and it was a good exercise. There may be some differences in what is possible to instrument depending on the available API for instrumenting e.g. an HTTP client. If we want to adapt this to that, it may make sense in the future to add a feature to configure capabilities of an instrumentation that enables or disables tests for those capabilities. |
Introduce test suite for HTTP client instrumentations to check that all implementations have the naming and tags expected. This may also help instrumentors ensure backwards compatibility of the HTTP client instrumentation across changes to it.
Avoids putting WireMock classes in the API that instrumentors need to implement for the test suite. Also makes it more versatile by passing a method, templated path, and variable substitutions. How templated paths are handled will vary from client to client.
Each implementation has been configured to tag with the URI pattern and that is checked now. Unmapped paths, on the other hand, should not be tagged as requested in the URI. This is currently failing for the Apache HTTP instrumentation.
Rename classes to be standard test class names. Minimize visibility. Remove 404 test, add a disabled test for requests to a down server. We will need to improve the existing instrumentations to add more tests of expected behavior such as that test.
We want to test more than the GET method, and some HTTP clients require a body for POST requests.
|
Since we want to verify existing behavior is maintained, it makes more sense to me to merge this into the oldest supported branch. Rebasing to merge into 1.8.x instead of 1.9.x |
shakuzen
force-pushed
the
http-client-instrumentation-tck
branch
from
fd51a2d
to
b5c3e49
Compare
Introduce test suite for HTTP client instrumentations to check that all implementations have the naming and tags expected. This may also help instrumentors ensure backwards compatibility of the HTTP client instrumentation across changes to it.