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

Use default skip pattern for MP metrics, openapi, health #95

Merged
merged 3 commits into from
Sep 10, 2018

Conversation

pavolloffay
Copy link
Member

Resolves #89

Signed-off-by: Pavol Loffay ploffay@redhat.com

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay
Copy link
Member Author

To test this I have added servlets mapped to /health, /openapi... I am not sure whether this is the right way to go. Maybe do not include any servlets and just add dependency on APIs and expect vendor adds implementation when running the tests. But doing so means vendor has to provide an implementation for MP health, openapi and metrics.

@pavolloffay
Copy link
Member Author

@kenfinnigan @Emily-Jiang could you please have a look at ^^

* @author Pavol Loffay
*/
@WebServlet(value = "/metrics/*")
public class MetricsServlet extends HttpServlet {
Copy link
Member Author

Choose a reason for hiding this comment

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

e.g. I test metrics exclusion by adding this servlet

@@ -192,6 +192,8 @@ Configuration of the skip pattern leverages MicroProfile Config specification.
The skip pattern is specified as a string with key `mp.opentracing.server.skip-pattern` which has to be
compliant with `java.util.regex.Pattern`. An example skip pattern might be `mp.opentracing.server.skip-pattern=/health|/metrics.*`

The default value of the skip pattern excludes tracing for endpoints defined by MicroProfile Health, Config and OpenAPI specification.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "Metrics" and not "Config"?

Copy link
Member Author

Choose a reason for hiding this comment

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

lol I don't know what I wanted to exclude in config

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@kenfinnigan
Copy link
Contributor

I think it's fine to add a servlet to test that the skip pattern works

@pavolloffay
Copy link
Member Author

@fmhwong could you please also have a look?

@@ -80,6 +81,7 @@ public static WebArchive createDeployment() {

WebArchive war = ShrinkWrap.create(WebArchive.class, "opentracing.war")
.addPackages(true, OpenTracingClientBaseTests.class.getPackage())
.addClass(HealthServlet.class)

Choose a reason for hiding this comment

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

Shouldn't the metric and openapi servlets be added aswell?

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 will remove this. There are added in addPackages and then auto-discovered. This was just my debug attempt :)

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@fmhwong
Copy link
Contributor

fmhwong commented Sep 8, 2018

LGTM

@pavolloffay pavolloffay merged commit 13609e7 into eclipse:master Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants