- Notifications
You must be signed in to change notification settings - Fork686
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
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
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
| functiongetReleaseVersion():string{ | ||
| letversion='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.
Uh oh!
There was an error while loading.Please reload this page.
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 thepackage.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.jsonscenarios 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 | AddedgetReleaseVersion() and includedversion 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
packageFilePathis undefined in this context. It should be replaced withversionFilePathto read the mockpackage.jsoncorrectly.
const originalPackageData = fs.existsSync(versionFilePath) ? fs.readFileSync(packageFilePath, 'utf-8') : null;| } | ||
| functionsetContext(context:Context,module?:string){ | ||
| logger.addPersistentLogAttributes({ | ||
| 'aws-request-id':context.awsRequestId, | ||
| 'function-name':context.functionName, | ||
| version:getReleaseVersion(), |
CopilotAIJul 4, 2025
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] CallinggetReleaseVersion() on every invocation performs synchronous file I/O repeatedly. Consider reading and caching the version once at module load to avoid repeated filesystem access.
| } | |
| functionsetContext(context:Context,module?:string){ | |
| logger.addPersistentLogAttributes({ | |
| 'aws-request-id':context.awsRequestId, | |
| 'function-name':context.functionName, | |
| version:getReleaseVersion(), | |
| })(); | |
| functionsetContext(context:Context,module?:string){ | |
| logger.addPersistentLogAttributes({ | |
| 'aws-request-id':context.awsRequestId, | |
| 'function-name':context.functionName, | |
| version:releaseVersion, |
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,
letreleaseVersion:string|null=null;functionsetContext(context:Context,module?:string){if(releaseVersion===null){releaseVersion=getReleaseVersion();}// ...}
but yeah probably minor, does look like it's only called once per function.
| // Add the context to all child loggers | ||
| childLoggers.forEach((childLogger)=>{ | ||
| 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'tmatter 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(); | ||
| constloggerModule=awaitimport('../'); | ||
| 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 realpackage.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. MakegetReleaseVersion() take a directory, like this:
functiongetReleaseVersion(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.
| } | ||
| functionsetContext(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,
letreleaseVersion:string|null=null;functionsetContext(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 has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request introduces changes to improve release management and enhance the lambda's loggin.
Release updates lambda
package.jsonfile 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)`)