Update OpenTracing docs based on further review comments in original PR #530
Conversation
|
Sorry, I merged the original PR to be sure to get it in the release, let me go through that one more deeply. |
|
@cescoffier I did a proper review and tested the guide so I think we are good. This PR is the result of my review. |
| @@ -10,7 +10,7 @@ To complete this guide, you need: | |||
| * less than 15 minutes | |||
| * an IDE | |||
| * JDK 1.8+ installed with `JAVA_HOME` configured appropriately | |||
| * Apache Maven 3.5+ | |||
| * Apache Maven 3.5.3+ | |||
There was a problem hiding this comment.
Can you also add Docker as it seems required.
|
|
||
| NOTE: Currently the tracer can only be configured to report spans directly to the collector via HTTP, using the `JAEGER_ENDPOINT` property. Support for using the Jaeger agent, via UDP, will be available in a future version. | ||
|
|
||
| Once both the application and tracing system are started, you can make a request to the provided endpoint: | ||
|
|
||
| ``` | ||
| $ curl http://localhost:8080/app/hello |
There was a problem hiding this comment.
Did you try without the application class and so remove the app prefix.
There was a problem hiding this comment.
No - just used the classes as generated by shamrock maven plugin.
There was a problem hiding this comment.
No problem, it's a "new" feature.
There was a problem hiding this comment.
@objectiser to be more clear, if you remove the ShamrockApplication class in the quickstart, it should expose your endpoint with the app/ prefix so you should do that and update the URL here in the guide.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah sorry, misunderstood - will manually do the change and retest.
There was a problem hiding this comment.
@cescoffier @gsmet Removed the ShamrockApplication class and tried curl http://localhost:8080/hello but got Not Found html page.
There was a problem hiding this comment.
Another reason to add a page listing all routes on Mode == Mode.DEV
There was a problem hiding this comment.
Would it be better to merge this as is, and then update all quickstarts in one go, when #510 is merged?
|
@gsmet yes, saw that! Thanks! I've found 2 minor issues. |
|
I rebased, incorporated the Docker requirement in your commit and force-pushed. Will merge now. |
* fix: fail early if using --native with .jsh Fixes quarkusio#510 * fix: jbang --interactive test.jsh no longer fails on empty on file. Fixes quarkusio#465 * fix: --native x.jsh prints warning instead of hard error.
Related to #521