HPCC-32982 Add alternative span scopes & OwnedSpanScope refactoring #19342
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32982 Jirabot Action Result: |
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32982 Jirabot Action Result: |
| @@ -159,11 +159,70 @@ interface ISpan : extends IInterface | |||
| virtual const char* queryLocalId() const = 0; | |||
| }; | |||
|
|
|||
| //------------------------------------------------------------------------------ | |||
There was a problem hiding this comment.
I felt ActiveSpanScope and OwnedActiveSpanScope better explained what these classes are doing but I am certainly open to changing the names.
system/jlib/jtrace.hpp
Outdated
| // involving multiple threads, time sliced work, etc ActiveSpanScope should be used. | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| class ActiveSpanScope |
There was a problem hiding this comment.
Add a comment that the span must be guaranteed to be owned by another class while this executes.
system/jlib/jtrace.hpp
Outdated
| // involving multiple threads, time sliced work, etc ActiveSpanScope should be used. | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| class ActiveSpanScope |
There was a problem hiding this comment.
needs class jlib_decl, otherwise you will get link failures when symbols are not exported.
system/jlib/jtrace.hpp
Outdated
| Owned<ISpan> span; | ||
| ISpan * prevSpan = nullptr; | ||
| }; | ||
|
|
||
| class jlib_decl OwnedSpanScope |
There was a problem hiding this comment.
Suggest reordering this before OwnedActiveSpanScope, so there is a logical progression of functionality within the classes.
There was a problem hiding this comment.
I almost wonder if OwnedSpanLifetime would be a better name - since it isn't really associated with a scope.
system/jlib/jtrace.cpp
Outdated
| ISpan* current = queryThreadedActiveSpan(); | ||
| if (current != span) | ||
| { | ||
| const char* currSpanID = current != nullptr ? current->querySpanId() : "null"; |
There was a problem hiding this comment.
minor: queryThreadedActiveSpan() can never be null, but CNullSpan::querySpanId() should be changed to return "null".
system/jlib/jtrace.hpp
Outdated
| // OwnedActiveSpanScope controls the lifetime of its ISpan while ActiveSpanScope | ||
| // does not. In cases where the ISpan will be used from a single thread and within | ||
| // a single scope OwnedActiveSpanScope should be used. For more complicated scenarios, | ||
| // involving multiple threads, time sliced work, etc ActiveSpanScope should be used. |
There was a problem hiding this comment.
Add an extra comment? (Do you agree with it?)
involving multiple threads, time sliced work, ...
a OwnedSpanScope should be stored as a member in a class, and an ActiveSpanScope used to associate that span with each processing thread.
…toring - Added ActiveSpanScope to track only current active span - Renamed existing OwnedSpanScope to OwnedActiveSpanScope - Added OwnedSpanScope to control only span lifetime Signed-off-by: James McMullan [email protected]
|
@ghalliday squashed |
ghalliday
merged commit 0bb5480
into
hpcc-systems:candidate-9.8.x
|
Jirabot Action Result: |
Signed-off-by: James McMullan [email protected]
Type of change:
Checklist:
Smoketest:
Testing: