Repository navigation
refactor(logger): isolate log level InvokeStore access in a store - #5548
Conversation
Follow-up to aws-powertools#5542: move all InvokeStore/async-context interaction for the per-invocation log level into a dedicated LogLevelStore class, mirroring the LogAttributesStore and LogBufferStore abstractions, so the Logger no longer reaches into the InvokeStore directly. Behavior is unchanged: the debug sampling decision is scoped per invocation under concurrency and the base level (set at initialization or via Advanced Logging Controls) is the fallback.
|
A store just for the log level seems overkill, I wonder can we put the buffer and log level in the same store as they are quite related. The log attributes stuff makes sense to be logically separated though I think we could make an argument that there could just be one store for all async context related stuff. That however would be a bigger change here that could be. done later as none of these stores are public facing. |
|
Good point @svozza, I'll consolidate. Rather than a dedicated LogLevelStore, I'll move the log-level scoping into the same store as the buffer from #5543, keeping LogAttributesStore separate which matches your "buffer and level are related" grouping. I'd suggest letting #5543 land first (it's the bug fix), then reworking this PR on top to fold the level into that store (renamed to cover both), which also avoids the churn of introducing a store here that then gets absorbed. The single-store-for-all-async-context idea makes sense as a later, separate change. What do you think? |
|
Agreed on mergeing #5543 first and also that we need to rework that PR too so it has a more generic store name. And yes, store consolidation is out of scope here too. |
|
@vishwakt the other PR is merged, please rebase & resolve conflicts when ready. |
…vel-store # Conflicts: # packages/logger/src/Logger.ts
Per review, consolidate the per-invocation log level into the existing LogInvocationStore instead of a dedicated store, keeping LogAttributesStore separate. The store is now created eagerly (it always holds the log level) and its buffer is enabled via configureBuffer() when log buffering is turned on. Logger delegates every log-level read and write to the store and no longer accesses the InvokeStore directly. Behavior is unchanged: the debug sampling decision is scoped per invocation under concurrency, with the base level as the fallback.
|
@svozza @dreamorosi , this one's ready for review |
|
@vishwakt I think your agent might've mistakenly overwritten the PR body, ask it to restore its format please. We'll review later today! |
|
Do we need a concurrency test that specifically targets log levels? There's quite a bit of conditional logic here that has the added complication of being dependent on the absence/presence of the invoke store. I want to make sure we're covered here. |
Add a concurrency test that interleaves two invocations each calling setLogLevel, asserting neither clobbers the other's effective level, to explicitly cover the log level scoping under LMI concurrency.
|
@svozza Log-level scoping under concurrency is covered in tests/unit/concurrency/sampling.test.ts, which exercises exactly the InvokeStore presence/absence branches you flagged:
I've also added a test that interleaves two concurrent invocations each calling setLogLevel and asserts neither clobbers the other's effective level. Also, I confirmed it fails if the level is stored as a shared instance field and passes with the per-invocation store, so it's guarding the actual behavior. |
|
Is there a reason why we didn't use the |
Use the shared sequence utility from the testing package instead of hand-rolled barriers, matching the other concurrency tests.
|
No good reason, I just hadn't reached for it. Switched to sequence in the latest commit. the sampling tests still use explicit store.run() because they need specific X-Ray trace IDs in the context, and sequence runs invocations with an empty context. Happy to extend sequence to accept a per-invocation context object if you'd like those converted too. |
Oh good shout! Yeah, let's do this, it's a useful addition. If the context is not provided, we can fall back to an empty object. |
Add an optional `context` to each invocation passed to `sequence`, used as the InvokeStore context for that invocation (e.g. an X-Ray trace id) and defaulting to an empty object. This lets concurrency tests that depend on distinct per-invocation trace ids use the shared helper instead of hand-rolling InvokeStore.run().
…ncy tests Convert the log buffer and debug sampling concurrency tests to the shared sequence helper, passing each invocation its own X-Ray trace id via the new context option, instead of hand-rolling InvokeStore.run() with barriers.
|
Thank you for your work on this @vishwakt, the PR is in a great state now. Merging! |
Summary
Changes
Follow-up to the review discussion on #5543: the per-invocation log level scoping (added in #5542 / #5544) accessed the
InvokeStoredirectly from withinLogger. Per @svozza's feedback, rather than a dedicated store this folds the log level into the existingLogInvocationStore(introduced for the buffer in #5543), keepingLogAttributesStoreseparate.LogInvocationStorenow also owns the effective log level:getLogLevel()/setLogLevel()resolve the invocation-scoped level when a context is active, andgetBaseLogLevel()/setBaseLogLevel()operate on the base level (set at initialization and for Advanced Logging Controls, which apply environment-wide).configureBuffer()when log buffering is turned on.Loggerdelegates every log-level read and write to the store and no longer references theInvokeStoredirectly for either the level or the buffer.Pure refactor, no behavior change: the existing concurrency, sampling, and buffer suites pass unchanged (231 logger tests), with 100% coverage on
Logger.tsandLogInvocationStore.ts.This branch merges
main(which has #5543) and resolves the conflict, so no rebase is needed.Issue number: fixes #5542
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.