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

Opentracing it #18169

Merged
merged 2 commits into from
Aug 23, 2021
Merged

Opentracing it #18169

merged 2 commits into from
Aug 23, 2021

Conversation

loicmathieu
Copy link
Contributor

This PR creates a Smallrye OpenTracing integration test (inspired by the OpenTelemetry one) and include a specific test for the JDBC instrumentation.

This is still a work in progress as the JDBC span is not created, but the test crash if we use the current JDBC driver version and pass if we use version 2.0.4 so it might have catch #18033 if we had it previously.

@loicmathieu
Copy link
Contributor Author

@kenfinnigan if you can have a look at it, it's still a draft as I need to find why the JDBC span is not created.

Copy link
Member

@kenfinnigan kenfinnigan left a comment

Choose a reason for hiding this comment

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

Seems reasonable from a first look through

@kenfinnigan
Copy link
Member

@loicmathieu Have you seen #18313? Are you ok with it?

@loicmathieu
Copy link
Contributor Author

@kenfinnigan no, I'm OK to merge my changes on the new IT created for Zipkin compatibility when merged.
So let's just merge #18313 when you're OK with it, I'll rebase my changes on top of it.

@loicmathieu
Copy link
Contributor Author

@kenfinnigan I rebase my PR onto main now that zipkin compatibility mode has been merged.
I choose to remove the existing IT that only test zipkin compatibility as my IT test way more functionality and both are not compatible (I use the mock support for easier testing).

If you think testing zipkin compatibility is important, I can copy the existing test and include it as a QuarkusUnitTest inside the deployment module of quarkus-jaeger.

@kenfinnigan
Copy link
Member

If you could add the Zipkin compatibility test back in the deployment I think that would be good. Thanks

@loicmathieu
Copy link
Contributor Author

@kenfinnigan

If you could add the Zipkin compatibility test back in the deployment I think that would be good.

OK, will do it

@loicmathieu
Copy link
Contributor Author

@loicmathieu loicmathieu marked this pull request as ready for review August 23, 2021 08:11
@loicmathieu loicmathieu merged commit f7177a9 into quarkusio:main Aug 23, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Aug 23, 2021
@loicmathieu loicmathieu deleted the opentracing-it branch August 23, 2021 08:42
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.

2 participants