[WOR-1560] WSM uses Stairway 1.0.0's native MDC support #1666
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
This is now enabled as needed via TCL's terra.common.tracing.stackdriverExportEnabled property.
TCL upgrade will have removed it. All of our AWS functionality is unsupported, so default to disabled. Ticket to fully clean up FeatureService and related code: WOR-1559
This required OTEL dependency upgrades for bootRun to work, following DataBiosphere/terra-common-lib#131 Is there a better way to get these from TCL without redefining them downstream?
MdcHook is greatly reduced and renamed to StairwayLoggingHook: it now only logs at notable state transitions.
This now matches the integration version as well as the version in TCL.
okotsopoulos
commented
Mar 18, 2024
okotsopoulos
requested review from
cahrens and
aherbst-broad
and removed request for
a team
aherbst-broad
approved these changes
Mar 19, 2024
blakery
approved these changes
Mar 19, 2024
I learned that Spring Boot Dependency Manager pulls in opentelemetry-bom as a dependency. For our Spring Boot version 3.1.2, opentelemetry-bom has version 1.25.0. Using the BOM (bill of materials) to manage OTEL dependencies is preferred to managing them individually: the BOM makes sure that they remain compatible with one another. The only OTEL dependencies that WSM needs to define directly are: 1. opentelemetry-api so that we can construct OpenTelemetry objects (versioned by the BOM, which is itself versioned by Spring Boot dependency manager) 2. opentelemetry-instrumentation-annotations so that we can use the @WithSpan annotation terra-common-lib defines the OTEL deps it needs, they are available as runtime dependencies so we don't need to redefine them. But it does not use the BOM to version them, or Spring Boot dependency manager. It versions them directly at 1.34.1. I found when removing the OTEL deps that WSM didn't need, I also needed to instruct our Spring Boot dependency manager to pin opentelemetry-bom at a higher version compatible with TCL's OTEL dependencies.
okotsopoulos
force-pushed
the
okotsopo-WOR-1560-stairway-native-mdc
branch
from
eda6738
to
8aef8d6
Compare
|
Please retry analysis of this Pull-Request directly on SonarCloud |
After merging in the main branch, I noticed that our Google BOM versions have again diverged. There is a wider mystery here as to why Dependabot is not picking up on some dependencies, but for now, bringing this one up to date.
|
|
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.
https://broadworkbench.atlassian.net/browse/WOR-1560
Objective
WSM uses Stairway 1.0.0's native MDC (mapped diagnostic context) support, rather than populating the log MDC itself on flight state transitions.
How?
To achieve this, I first upgraded to the latest terra-common-lib, from which we get our Stairway dependency. Then I was able to drastically simplify our
MdcHook: it is nowStairwayLoggingHook, and does what it says on the tin (supplement logging on notable state transitions). I removed now-unused code, including cleaning upJobBuilder's job initialization which no longer has to serialize the calling thread's MDC and write it to the input map.Because this TCL version got rid of Flagsmith, and WSM used Flagsmith in its user-level feature flagging (a Verily artifact), I excised Flagsmith from our repo to allow this upgrade and ticketed further clean-up here (we discussed in refinement that we wanted to fully remove AWS code from the repo): https://broadworkbench.atlassian.net/browse/WOR-1559
Other Dependency Upgrades
I had some trouble with dependencies straight out of the box: OTEL dependencies, and a Google BOM conflict within our repo.
I learned that Spring Boot Dependency Manager for Spring Boot 3.1.2 pulls in opentelemetry-bom:1.25.0 as a dependency.
Using the BOM (bill of materials) to manage OTEL dependencies is preferred to managing them individually: the BOM makes sure that they remain compatible with one another.
The only OTEL dependencies that WSM needs to define directly are:
opentelemetry-apiso that we can constructOpenTelemetryobjects (versioned by the BOM, which is itself versioned by Spring Boot dependency manager)opentelemetry-instrumentation-annotationsso that we can use the@WithSpanannotation (not versioned by the BOM)terra-common-lib defines the OTEL deps it needs, they are available as runtime dependencies so we don't need to redefine them. But it does not use the BOM to version them, or Spring Boot dependency manager... it versions them directly at 1.34.1 (and 2.0.0 for instrumentation).
I found when removing the OTEL deps that WSM didn't need, I also needed to instruct our Spring Boot dependency manager to pin opentelemetry-bom to a higher version compatible with TCL's OTEL dependencies.
Demonstration
I deployed the latest version of this feature branch to my Bee, and triggered a flight. The resulting logs have their
jsonPayloadpopulated as we'd expect:jsonPayload.requestIdcomes from)A
jsonPayloadfor a log emitted during a step:All logs for the request, with
flightIdandflightStepNumberadded to the summary line: