Utility to create spans for synchronous blocks of code #109467
Conversation
idegtiarenko
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| var ctx = threadPool.getThreadContext().newTraceContext(); | ||
| tracer.startTrace(threadPool.getThreadContext(), span, name, attributes); |
There was a problem hiding this comment.
First off: this is great and allows to create sub-spans of the current thread context very easily.
However, I wonder if we should use threadPool.getThreadContext().newTraceContext(); this mutates the current thread local state, and in particular only works if there's a strict hierarchy of spans. @elastic/es-core-infra what do you think?
It might be sufficient to copy the logic from newTraceContext to create a new, separate TraceContext and store it as part of the Releaseable that is returned here. I attempted that here and it seemed to work fine.
This would allow create interleaved traces in the same thread which would provide more flexibility; my use case for this is seeing when individual ESQL operators start and stop, even though they are executed concurrently on the same thread.
There was a problem hiding this comment.
I am worried about the use of threadpool & threadcontext here.
we should aim to use org.eleasticsearch.telemetry.tracing.TraceContext
I think it should be possible to either:
- refactor the Pr to have the new context and context to restore (would be probably a bit ugly)
- refactor the TraceContext to create a newTraceContext
There was a problem hiding this comment.
refactor the Pr to have the new context and context to restore (would be probably a bit ugly)
Not sure I follow, threadPool.getThreadContext().newTraceContext() creates a new context and returns a releasable to rollback to the previous one.
To me it sounds like what is suggested, however implicit (may be the comment is about it?)
There was a problem hiding this comment.
refactor the Pr to have the new context and context to restore (would be probably a bit ugly)
as part of a method signature.
My point is that we should not try to add ThreadContext in the telemetry api
pgomulka
left a comment
pgomulka
left a comment
There was a problem hiding this comment.
I am not convinced we should do this like this. This will be only usable in server, would we like to use it in plugins too? (currently possible due to this api living in server, but this was just a shortcut we did in the past)
| var ctx = threadPool.getThreadContext().newTraceContext(); | ||
| tracer.startTrace(threadPool.getThreadContext(), span, name, attributes); |
There was a problem hiding this comment.
I am worried about the use of threadpool & threadcontext here.
we should aim to use org.eleasticsearch.telemetry.tracing.TraceContext
I think it should be possible to either:
- refactor the Pr to have the new context and context to restore (would be probably a bit ugly)
- refactor the TraceContext to create a newTraceContext
| import org.elasticsearch.core.Releasable; | ||
| import org.elasticsearch.telemetry.tracing.Traceable; | ||
| import org.elasticsearch.telemetry.tracing.Tracer; | ||
| import org.elasticsearch.threadpool.ThreadPool; |
There was a problem hiding this comment.
so I am planning to move the telemetry package into its own :lib this import will make it impossible.
5 participants
This change contains a small utility that allows to create a sub-spans for synchronous blocks of code that do not require task registration for them. The change itself is not adding any usages. They are going to be implemented separately. Utility could be used as following:
I have couple of example usages in #109244