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

Adds dedicated tests for OC shim #7488

Merged
merged 11 commits into from
May 5, 2023

Conversation

dmarkwat
Copy link
Contributor

Per suggestion: open-telemetry/opentelemetry-java#4900 (review)

Note: these tests won't succeed until a new version of upstream is published with the requisite changes to make the opencensus shim work under the otel javaagent.

Happy to add more tests to this. However, until upstream is released, that's mostly eyeballing everything, so will wait for a sign that the current state of upstream is sufficient to move forward before spending the time.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

👍

@dmarkwat
Copy link
Contributor Author

The tests won't pass until open-telemetry/opentelemetry-java#4900 is merged, but the cases should largely be covered.

@dmarkwat dmarkwat force-pushed the oc-shim-dedicated-tests branch 2 times, most recently from 4c3c412 to c278c14 Compare March 11, 2023 01:46
@dmarkwat
Copy link
Contributor Author

Rebased on main and built against the recently-released opentelemetry-java 1.24.0 and all tests passing as expected 👍 . Waiting for the javaagent to get released and this should begin compiling and tests passing in CI.

@dmarkwat dmarkwat force-pushed the oc-shim-dedicated-tests branch 2 times, most recently from 869bd34 to d7f185d Compare March 15, 2023 23:28
@dmarkwat
Copy link
Contributor Author

Rebased on main for version 1.24.0 of otel-java and looking good, apart from the one outlying comment wrt the opencensus-shim-generated instrumentation name, mentioned here.

@dmarkwat
Copy link
Contributor Author

dmarkwat commented Apr 4, 2023

Should this have been closed? Or was it github automation? I still think this is required to adequately test the OC shim running under the javaagent.

cc @jack-berg

@laurit
Copy link
Contributor

laurit commented Apr 4, 2023

@dmarkwat github automatically closes issues and pull requests when a pull request that says something like resolves #123 is merged.

@laurit laurit reopened this Apr 4, 2023
@dmarkwat
Copy link
Contributor Author

Rebased and should be good to go 👍

@mateuszrzeszutek mateuszrzeszutek added this to the v1.26.0 milestone May 5, 2023
@trask trask merged commit aef70d2 into open-telemetry:main May 5, 2023
@trask
Copy link
Member

trask commented May 5, 2023

thx @dmarkwat for your efforts and patience!

@dmarkwat
Copy link
Contributor Author

dmarkwat commented May 5, 2023

Cheers, i was happy to do it. And thanks for your patience and guidance as well. I hope to have more contributions for this awesome project soon!

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.

4 participants