[DCJ-507] Stairway with work queue enabled should be context-aware #152
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context-aware Stairway previously assumed that the calling thread would have mapped diagnostic context (MDC) to persist to the child threads spawned for flight execution. This was not the case when Stairway is configured with a work queue enabled (e.g. GCP Pub-Sub). Here, flight information was written to the pub-sub topic without the calling thread's context, so when a message was processed and deserialized any logs emitted from that flight would be missing context set by the original calling thread (e.g. request ID). - Expanded QueueMessageReady to store calling thread context on construction - QueueMessageReady.process sets the MDC using its stored calling thread context, which then makes the flight context-aware - Expanded unit tests
okotsopoulos
requested review from
a team,
rushtong and
fboulnois
and removed request for
a team
pshapiro4broad
approved these changes
Jul 12, 2024
stairway/src/main/java/bio/terra/stairway/queue/QueueMessageReady.java
Outdated
Show resolved
Hide resolved
|
|
||
| @BeforeEach | ||
| void beforeEach() { | ||
| MDC.clear(); |
There was a problem hiding this comment.
Would it be better to use a static mock instead of the actual MDC API? We don't use mocks yet for MDC, so it may be too big a change for this PR, but it's something to consider if we'd like to isolate our tests from the MDC implementation, and might make it easier to check if our code is doing what we expect.
There was a problem hiding this comment.
Hmm, yeah this feels like too big a change for this PR. But you make good points about the benefits of test isolation.
MdcUtils: - Added public utility method callWithContext for running and returning a callable with MDC's context map temporarily overwritten - Remove public modifier on overwriteContext in favor of the above QueueMessageReady: - Use MdcUtils.callWithContext to simplify process definition - Revert process return strategy to return booleans directly rather than setting a boolean to return later - Remove unnecessary callingThreadContext setter (left existing setters untouched even though they can likely be removed: OOS) - Left public callingThreadContext getter unchanged: it is required to be public for serde operations Added unit test coverage for all changes.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira ticket: https://broadworkbench.atlassian.net/browse/DCJ-507
Addresses
Context-aware Stairway previously assumed that the calling thread would have mapped diagnostic context (MDC) to persist to the child threads spawned for flight execution.
This was not the case when Stairway is configured with a work queue enabled (e.g. GCP Pub-Sub). Here, flight information was written to the pub-sub topic without the calling thread's context, so when a message was processed and deserialized any logs emitted from that flight would be missing context set by the original calling thread (e.g. request ID).
Where we've felt this impact in TDR:
jsonPayload.requestId) is slowing down investigative progress.Summary of changes
MdcUtils.callWithContextto call a callable and return its result with MDC's context map temporarily overwritten.QueueMessageReadyto store calling thread context on construction.QueueMessageReady.process()sets the MDC using its stored calling thread context, which then makes the flight context-aware.Testing Strategy
MdcUtils.callWithContext.QueueMessageReadywith special attention to MDC behavior and propagation in serialization, deserialization, and processing.Note: Sonarcloud coverage stats are incorrectly reporting 0% coverage. They look to have never worked. I do believe I've covered all of my changes in unit tests.ETA: thanks @pshapiro4broad for fixing the Sonar scans, I pulled the fix into this branch and we are now seeing coverage stats.