Skip to content
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

feat: more logs for extension and notification apps #1183

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

Krysik
Copy link
Contributor

@Krysik Krysik commented Jul 11, 2024

Summary

The motivation behind this PR is that I would like to observe better what's going on in the extension and notification applications. I would like to track a whole request path by request id and have a response time.

In my PR I inject a logger instead of importing it from utils. The injected logger has a context of the request, it contains information about the requesting path, used HTTP method, and request id. In my opinion, it's better to have a context instead of a raw logger without anything.

Also, I changed the notification handler to accept an object. This approach is more flexible in case of adding new things in the future and it provides more context for the developer, especially for the boolean values, now you don't have to remember the order of the arguments.

Tested scenarios

Not much to test here, I added logs 😉😄

@Krysik Krysik changed the title feat: more logs for extension and notification application feat: more logs for extension and notification apps Jul 12, 2024
@Krysik Krysik force-pushed the feat/improve-logs branch 3 times, most recently from a5bb908 to a677e53 Compare July 17, 2024 06:39
@Krysik
Copy link
Contributor Author

Krysik commented Jul 18, 2024

@jhilden @lojzatran @emmenko @logeecom, Will some of you have some time to look at it?

@logeecom
Copy link
Contributor

logeecom commented Aug 6, 2024

Hello @Krysik

Our team will look into this and get back to you.

Best regards.

@Krysik
Copy link
Contributor Author

Krysik commented Sep 3, 2024

@logeecom any update on this PR?

@tamaralogeecom tamaralogeecom merged commit 9fc51c8 into Adyen:master Sep 27, 2024
@adyen-integrations-support

Hey @Krysik

Pull request #1183 is merged.

We have reviewed the changes and tested them.

This will be in the next release.

Best regards.

@adyen-integrations-support

Hey,

New version 11.5.7 has been released https://github.com/Adyen/adyen-commercetools/releases/tag/v11.5.7

Best regards.

notification,
enableHmacSignature,
ctpProjectConfig,
) {
const logger = mainLogger.child({
logger
Copy link

@vasily-polonsky vasily-polonsky Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaking change, that parameter is not provided for serverless setup, for example aws lambda (notification/index.lambda.js)

@adyen-integrations-support

Hey @vasily-polonsky

We wanted to inform you that our team has added the logger parameter for serverless and resolved the issue.

The new plugin version 11.5.10 has been released https://github.com/Adyen/adyen-commercetools/releases/tag/v11.5.10.

Best regards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants