Add ExecInitializer #9
Conversation
| @@ -27,8 +27,12 @@ | |||
| * which do not "inherit" registries from the calling execution. Consequently, | |||
| * the trace context must be passed explicitly to the forked executions. | |||
| * | |||
| * @deprecated As of 2.4 there is now a {@link ratpack.exec.ExecInitializer} provided by {@link ServerTracingModule} | |||
There was a problem hiding this comment.
Should this be 1.4 and not 2.4?
There was a problem hiding this comment.
The version is 2.3.2 now right?
There was a problem hiding this comment.
Ha yeah right. I was thinking Ratpack version here.
Since this wasn't available in older versions of Ratpack, maybe it would help to include the Ratpack version as well.
As a suggestion:
@deprecated As of 2.4 since Ratpack 1.4 and later has {@link ratpack.exec.ExecInitializer} which we register an instance of through {@link ServerTracingModule}? Just a suggestion for folks that read to quickly ;)
| /** | ||
| * ExecInitializer that will propagate the tracing context into any new execution created. | ||
| */ | ||
| public class TracingPropagationExecInitializer implements ExecInitializer { |
There was a problem hiding this comment.
I wonder if this could be a static inner class of RatpackCurrentTraceContext. Mostly suggesting this so we can continue keeping TraceContextHolder private. Honestly I'm not 100% sure that will work but just trying to see if we can not leak access to the otherwise private class.
There was a problem hiding this comment.
I don't know if we should keep TraceContextHolder private it makes it very painful to get in and mess with things which sometimes is needed.
If you want to try and keep everything locked in I am happy to do that.
There was a problem hiding this comment.
Making things harder to mess with is precisely why I suggest keeping it private. It's not to say that I'm suggesting making folks life harder but rather protecting the project from future difficulties. As soon as its accessible then it becomes part of the API and not something we can easily make breaking changes to in the future. Marking this as protected is certainly an option if others feel it is the right course of action as well :)
|
Thanks for working on this @beckje01! This will be awesome to finally not have to explain why TracedParallelBatch exists! :D |
To the ServerTracingModule add an ExecInitializer that will propagate tracing context across executions this solves for the Batch usecase. Bumped the minor version as the changes are new features but should not require code change from the user but does now depend on Ratpack 1.6
To the ServerTracingModule add an ExecInitializer that will propagate tracing context across executions this solves for the Batch usecase.
Bumped the minor version as the changes are new features but should not require code change from the user but does now depend on Ratpack 1.6