-
Notifications
You must be signed in to change notification settings - Fork 664
feat: Add release version to lambda conext for logging #4655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
* fix: add lambda version to context logging * release tests * release tests * fix: Add lambda version to log context * fix: Add lambda version to log context * fix: Add lambda version to log context * fix: Add lambda version to log context * fix: Add lambda version to log context * feat: Add lambda version to log context
.release-please-manifest.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is mandatory when using a release manifest
const defaultValues = { | ||
region: process.env.AWS_REGION, | ||
environment: process.env.ENVIRONMENT || 'N/A', | ||
}; | ||
|
||
function getReleaseVersion(): string { | ||
let version = 'unknown'; | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inention is here to avoid an excetion can crash the lambda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds release version management by reading the package.json
version in the logger and propagating it into the AWS Lambda log context, and configures the release-please action to update that version across multiple Lambda packages.
- Introduce
getReleaseVersion()
in the logger to read and persist the version attribute. - Add tests covering missing and present
package.json
scenarios for the logger. - Configure
.release-please-manifest.json
,.release-please-config.json
, and update the release workflow to bump versions.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
lambdas/libs/aws-powertools-util/src/logger/index.ts | Added getReleaseVersion() and included version in log context |
lambdas/libs/aws-powertools-util/src/logger/logger.test.ts | Added tests and file setup/cleanup for version handling |
.release-please-manifest.json | Added manifest file to pin root package version for releases |
.release-please-config.json | Configured extra-files paths for all Lambda package versions |
.github/workflows/release.yml | Updated Release-Please action to use the new manifest and config |
Comments suppressed due to low confidence (1)
lambdas/libs/aws-powertools-util/src/logger/logger.test.ts:96
- The variable
packageFilePath
is undefined in this context. It should be replaced withversionFilePath
to read the mockpackage.json
correctly.
const originalPackageData = fs.existsSync(versionFilePath) ? fs.readFileSync(packageFilePath, 'utf-8') : null;
} | ||
|
||
function setContext(context: Context, module?: string) { | ||
logger.addPersistentLogAttributes({ | ||
'aws-request-id': context.awsRequestId, | ||
'function-name': context.functionName, | ||
version: getReleaseVersion(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Calling getReleaseVersion()
on every invocation performs synchronous file I/O repeatedly. Consider reading and caching the version once at module load to avoid repeated filesystem access.
} | |
function setContext(context: Context, module?: string) { | |
logger.addPersistentLogAttributes({ | |
'aws-request-id': context.awsRequestId, | |
'function-name': context.functionName, | |
version: getReleaseVersion(), | |
})(); | |
function setContext(context: Context, module?: string) { | |
logger.addPersistentLogAttributes({ | |
'aws-request-id': context.awsRequestId, | |
'function-name': context.functionName, | |
version: releaseVersion, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a good catch, but as far I know the context is only called once via pwoertools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a fair defensive thing to do,
let releaseVersion: string | null = null;
function setContext(context: Context, module?: string) {
if (releaseVersion === null) {
releaseVersion = getReleaseVersion();
}
// ...
}
but yeah probably minor, does look like it's only called once per function.
@@ -20,6 +37,7 @@ function setContext(context: Context, module?: string) { | |||
childLogger.addPersistentLogAttributes({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't new in this PR so doesn't matter for the review, but it caught my eye... Why do we have to store the child loggers? Should be possible to arrange that only the root logger is ever seen and so we don't need to repeat the attribute addition, right?
beforeEach(async () => { | ||
// Clear the module cache and reload the logger module | ||
vi.resetModules(); | ||
const loggerModule = await import('../'); | ||
logger = loggerModule.logger; | ||
setContext = loggerModule.setContext; | ||
|
||
// Ensure a clean state before each test | ||
if (fs.existsSync(versionFilePath)) { | ||
fs.unlinkSync(versionFilePath); | ||
} | ||
}); | ||
|
||
afterEach(() => { | ||
// Clean up after each test | ||
if (fs.existsSync(versionFilePath)) { | ||
fs.unlinkSync(versionFilePath); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be a bit concerned about messing with the real package.json
file here, what happens if tests get run in parallel, and the reinitialisaion which seems complex. I would consider doing this with dependency injection. Make getReleaseVersion()
take a directory, like this:
function getReleaseVersion(packageDir: string = __dirname): string {
// ...
}
and then each test could make its own temp dir and set it up how it wants.
It would make sense to still have one integration test which uses the real file under __dirname
, to ensure that there's always a version set in there.
} | ||
|
||
function setContext(context: Context, module?: string) { | ||
logger.addPersistentLogAttributes({ | ||
'aws-request-id': context.awsRequestId, | ||
'function-name': context.functionName, | ||
version: getReleaseVersion(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a fair defensive thing to do,
let releaseVersion: string | null = null;
function setContext(context: Context, module?: string) {
if (releaseVersion === null) {
releaseVersion = getReleaseVersion();
}
// ...
}
but yeah probably minor, does look like it's only called once per function.
This pull request introduces changes to improve release management and enhance the lambda's loggin.
Release updates lambda

package.json
file with the release version in the release PR. See example: cloud-scouts#15The version of the lambda is added to the log context.

(diffhunk://#diff-8e1eeb7f895c1c12c548abcc3b9bcf66106ea5366250af44a364db81b5384446R40)`)