Allow clearing the current Scope.

opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163

http://opentracing.io

Apache License 2.0

1.68k stars 344 forks source link

Allow clearing the current Scope. #313

Open carlosalberto opened 5 years ago

carlosalberto commented 5 years ago

Decided to take on being able to clear the current Scope, if any - this is done to prevent leaking Scope objects that were left around unintentionally.

If merged, this could become part of the next iteration of https://github.com/opentracing/specification/pull/122 ;)

@yurishkuro @adriancole @felixbarny @tylerbenson

coveralls commented 5 years ago

Coverage increased (+1.04%) to 76.225% when pulling e9868eee428a9fa5f3c517a1e41f8b3096834763 on carlosalberto:scope_manager_clear into ac3c666aa665adcb1d76bce52f38cadf63625670 on opentracing:v0.32.0.

felixbarny commented 5 years ago

Who is supposed that method in which circumstances?

sjoerdtalsma commented 5 years ago

@felixbarny

Who is supposed that method in which circumstances?

I can imagine some servlet filter right before returning the thread to the pool. Or a custom job scheduler, -again- also before returning a worker thread to the pool. i.e. the one who controls threads, but may not control the correct activation / closing of current scopes of all called code.

sjoerdtalsma commented 5 years ago

Long time ago in the times of application servers and class loaders, I even went as far to create a list of all static ThreadLocal uses in our entire codebase (including libraries) and remove() all of them after each web request. It was a hack, but it made several memory leaks disappear and gave us time to perform a more thorough analysis of the root cause(s).

carlosalberto commented 5 years ago

So it seems we have some agreement on this - any opinion @objectiser @yurishkuro @CodingFabian ?

CodingFabian commented 5 years ago

I do not fully understand the purpose. Is this a band aid that can be used whenever something unexpected happens? A util for libraries to clear up the mess a user might have created or something that has legitimate use by a user? users could just decide to not commit a span or create a new one.

While this api is technically no problem, I wonder what the guidelines for users are. How is this api intended to be used by end users.

carlosalberto commented 5 years ago

Hey @CodingFabian

So this is more of a preventive use case, I'd say, and is not expected to be used by final users most of the time (same as ScopeManager.active()).

Quoting @sjoerdtalsma:

I can imagine some servlet filter right before returning the thread to the pool

And the issue around https://github.com/opentracing-contrib/java-jaxrs/issues/109 could use this one.

carlosalberto commented 5 years ago

Hey @CodingFabian Did the reason given above sounds right to you? Let me know ;)

@yurishkuro @opentracing/opentracing-java-maintainers Opinion on this?