Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP server instrumentation TCK #3379

Merged
merged 2 commits into from Sep 12, 2022

Conversation

shakuzen
Copy link
Member

@shakuzen shakuzen commented Aug 31, 2022

Similar to the http client instrumentation tests, these tests are meant for http server instrumentation to ensure they include the expected tags in a minimal set of scenarios that should be supported.

Pure Jetty servlet instrumentation does not even tag uri since getting a templated URI is not generically possible at that level. Therefore the tests are disabled for Jetty server instrumentation.

There are probably more scenarios we should test in all instrumentations. Please let me know more to add. Improvements to the usability/API are also welcome.

@shakuzen shakuzen added enhancement A general enhancement module: micrometer-test An issue that is related to our TCK labels Aug 31, 2022
@shakuzen shakuzen added this to the 1.10 backlog milestone Aug 31, 2022
@shakuzen shakuzen force-pushed the http-server-tck branch 2 times, most recently from dda2ffe to 176ecad Compare August 31, 2022 11:13
}

@GET
@Path("hello/{name}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these strings can't be static finals in the test abstract class. That way you will be sure that you're using the proper strings. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be good to have some way to avoid typos and improve discoverability beyond the current setup of it being in the JavaDoc. I'm wondering if an enum might make it easy for implementors to find all the routes that should be implemented. I'm going to draft something and see if I can get some feedback on it.

Copy link
Member Author

@shakuzen shakuzen Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I tried making the enum and immediately realized that won't work for using here because the annotation needs a constant value, and the compiler doesn't recognize enum fields/methods as constant, even though they effectively are in this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See newly added InstrumentedRoutes class with string constants. WDYT?

Similar to the http client instrumentation tests, these tests are meant for http server instrumentation to ensure they include the expected tags in a minimal set of scenarios that should be supported.
@shakuzen shakuzen changed the title WIP HTTP server instrumentation TCK HTTP server instrumentation TCK Sep 1, 2022
@shakuzen shakuzen marked this pull request as ready for review September 1, 2022 07:16
@shakuzen shakuzen modified the milestones: 1.10 backlog, 1.8.10 Sep 12, 2022
@shakuzen shakuzen merged commit bd470ce into micrometer-metrics:1.8.x Sep 12, 2022
@shakuzen shakuzen deleted the http-server-tck branch September 12, 2022 07:48
izeye added a commit to izeye/micrometer that referenced this pull request Sep 27, 2022
shakuzen pushed a commit that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement module: micrometer-test An issue that is related to our TCK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants