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

Upgrade to io.jaegertracing version of Jaeger #472

Merged
merged 2 commits into from
Jan 11, 2019
Merged

Upgrade to io.jaegertracing version of Jaeger #472

merged 2 commits into from
Jan 11, 2019

Conversation

objectiser
Copy link
Contributor

@objectiser objectiser commented Jan 11, 2019

And move Jaeger into its own module to enable other OpenTracing compliant tracers to be used.

Have tested with the getting-started-native example, by adding the opentracing deployment dependency:

        <dependency>
            <groupId>org.jboss.shamrock</groupId>
            <artifactId>shamrock-opentracing-deployment</artifactId>
            <scope>provided</scope>
            <version>${shamrock.version}</version>
        </dependency>

NOTE: If the shamrock-opentracing-deployment is added with shamrock-jaeger-deployment dependency excluded, then it will use the OpenTracing NoopTracer.

Then starting a local Jaeger standalone instance:

docker run -e COLLECTOR_ZIPKIN_HTTP_PORT=9411 -p 5775:5775/udp -p 6831:6831/udp -p 6832:6832/udp -p 5778:5778 -p 16686:16686 -p 14268:14268 -p 9411:9411 jaegertracing/all-in-one:1.8

and running the app:

target/shamrock-quickstart-runner -DJAEGER_SERVICE_NAME=myservice -DJAEGER_SAMPLER_TYPE=const -DJAEGER_SAMPLER_PARAM=1 -DJAEGER_ENDPOINT=http://localhost:14268/api/traces

and then send requests to the app using:

curl http://localhost:8080/app/hello

Then view the trace in the Jaeger UI.

Once this PR is closer to being merged, I will add documentation.

There are some areas that need further work - not sure whether should be tackled in this PR or separate PRs?

Finally - was thinking about adding OT/jaeger example in the quickstarts - should it be included in an existing quickstart (e.g. getting-started-native) possibly as an extra profile or in its own one, e.g. with-tracing? Depends how you want the quickstarts to demonstrate individual features.

…s own module to enable other OpenTracing compliant tracers to be used.
import io.jaegertracing.internal.metrics.Gauge;
import io.jaegertracing.spi.MetricsFactory;

// TODO: Determine if there is a more automated way we can configure the metrics
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to integrate with MP Metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main goal will be to integrate with the metrics provider used with protean - if MP metrics is the best way to achieve that then yes.

Copy link
Member

Choose a reason for hiding this comment

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

MP Metrics is present, so that's likely the best and easiest option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a look and I think it should be straightforward to leverage MP-metrics for this.

@kenfinnigan
Copy link
Member

Why not have shamrock-opentracing-deployment depend on shamrock-jaeger-deployment so that a user doesn't need to remember to add both dependencies?

We want to be prescriptive, which means Jaeger. If a developer wants to use a different open tracing implementation it's not unreasonable to require them to exclude Jaeger from open tracing and then add their own.

@objectiser
Copy link
Contributor Author

@kenfinnigan Sounds reasonable - just wanted to allow for the possibility for users to use their preferred OT tracer.

@kenfinnigan
Copy link
Member

They can still use their own tracer, will making it easier for users following the prescriptive approach (Jaeger)

@objectiser
Copy link
Contributor Author

@kenfinnigan Are there any items on the issues list in the description that you think need to be dealt with in this PR? or can they all be handled in subsequent PRs?

The main difference between the functionality of this PR with the existing tracing support is that this uses the HttpSender instead of the UdpSender - so requires users to define the one additional system property/env var (JAEGER_ENDPOINT).

@kenfinnigan
Copy link
Member

I think they can all be done as subsequent tasks, so maybe create GH issues for them so we don't lose track of them.

One to add to the list would be supporting setting the JAEGER_ENDPOINT with config and not just a system property.

@kenfinnigan kenfinnigan merged commit 9791355 into quarkusio:master Jan 11, 2019
@objectiser objectiser deleted the opentracing branch January 11, 2019 19:16
@objectiser
Copy link
Contributor Author

@kenfinnigan Will do.

@cescoffier
Copy link
Member

@objectiser I see this PR merged. but none of the tasks from the checklist has been marked as done. Should we consider it done in 0.6?

@objectiser
Copy link
Contributor Author

@cescoffier Yes, it works fine as is. All the tasks in the checklist are improvements that will be done separately.

Although you may want to review/merge #521 which provide the docs for opentracing/jaeger - and once 0.6.0 is released, I will update quarkusio/quarkus-quickstarts#20 so the quickstart can be made available.

@cescoffier
Copy link
Member

@objectiser Thanks!

About the quickstarts, just open a PR against the development branch, so it will be released in a few hours.

@cescoffier cescoffier changed the title WIP: Upgrade to io.jaegertracing version of Jaeger and move Jaeger into it… Upgrade to io.jaegertracing version of Jaeger Jan 16, 2019
@cescoffier cescoffier added the kind/enhancement New feature or request label Jan 16, 2019
@cescoffier cescoffier added this to the 0.6.0 milestone Jan 16, 2019
@objectiser
Copy link
Contributor Author

@cescoffier Done quarkusio/quarkus-quickstarts#21

Its currently failing the tests - but I don't have access to the azure environment to check what the problem is. I tested it locally and it worked fine. Can I get access to azure?

@cescoffier
Copy link
Member

yes, the development branch fails because it can't access the snapshot artifacts.

maxandersen added a commit to maxandersen/quarkus that referenced this pull request Nov 5, 2022
+ upgraded gradle to have it compile with java 14/15.

Co-authored-by: Leandro Coutinho <lescoutinhovr@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants