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

Implement scope.close method #83

Conversation

pavolloffay
Copy link
Contributor

Related to: opentracing/opentracing-java#267 #82 and #74 (comment)

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

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

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

this programming pattern can cause misalignment bugs as it encourages people to close a scope they didn't open. I left a comment here opentracing/opentracing-java#267 (comment)

you can refer to serious users of tracing like @anuraaga and folks at LINE who spend hours trying to debug misalignment problems. please don't make apis that encourage bugs

@pavolloffay
Copy link
Contributor Author

this programming pattern can cause misalignment bugs as it encourages people to close a scope they didn't open. I left a comment here opentracing/opentracing-java#267 (comment)

I agree, however this should be changed in the OT API and not by not implementing the method correctly. The issues is that the current implementation does not work with many OT instrumentations:

@codefromthecrypt
Copy link
Contributor

sorry, the javadoc says this is only used to get the active span.

https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/ScopeManager.java#L39

that part is implemented and you guys have no tests. This issue will be left open to encourage you to fix the api.

@codefromthecrypt
Copy link
Contributor

by the way. you personally wrote or copied most of the instrumentations that don't work. You can fix those.

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