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

Expand gateway request logging in AWS, GCP, and airnode-client #1526

Merged
merged 9 commits into from
Nov 21, 2022

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Nov 10, 2022

Closes #1351.

User story for the motivation and context of this PR:

As a [developer], I want to [access request processing logs on any of GCP, AWS, or the airnode-client without needing knowledge of Airnode request handling], so jthat [I can debug failing requests].

@dcroote dcroote requested a review from a team November 10, 2022 07:56
Copy link
Contributor

@amarthadan amarthadan left a comment

Choose a reason for hiding this comment

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

I'm a bit skeptical about how much will this help with the debugging process. I'm not saying it's bad, it will definitely help but I don't know if it's enough. But it's a start 🙂

Comment on lines 120 to 122
// Unlike GCP, AWS automatically generates a unique request ID so no need to use one here
logger.info(`Received HTTP gateway request`);

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually necessary? I think every request (lambda invocation) is logged in the AWS (and I think also in GCP) anyway. I don't think this adds anything. I would at least set it to debug.

Copy link
Contributor Author

@dcroote dcroote Nov 19, 2022

Choose a reason for hiding this comment

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

Edit: I've kept it for all, but changed it to debug. I think keeping it aids in visualizing the beginning of the block of logs pertaining to the request.

packages/airnode-deployer/src/handlers/aws/index.ts Outdated Show resolved Hide resolved
packages/airnode-deployer/src/handlers/gcp/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@amarthadan amarthadan left a comment

Choose a reason for hiding this comment

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

I would use the log options with the meta field everywhere where you want to add some data to the log message. Similar to how it's done in Airseeker. Or do you feel like it's better this way?

@dcroote
Copy link
Contributor Author

dcroote commented Nov 18, 2022

I would use the log options with the meta field everywhere where you want to add some data to the log message. Similar to how it's done in Airseeker. Or do you feel like it's better this way?

Ah sorry it wasn't ready for re-review just yet 😅 I pushed that commit in order for CI to build the images that I could then test. But I think the meta option will be the better route 👍

@dcroote
Copy link
Contributor Author

dcroote commented Nov 19, 2022

I'm a bit skeptical about how much will this help with the debugging process.

Agreed, but it's a start and provides a lot more than what the user / Airnode operator gets access to currently. I actually found it useful in debugging encoded parameters for a signed gateway request 😅

I would use the log options with the meta field everywhere where you want to add some data to the log message

For the local client, a requestId gets generated and added as logging metadata (with meta: as you suggested) to distinguish requests, but I found this is not needed for AWS & GCP as each generates their own form of unique ID for an incoming request that groups logs nicely (RequestId and executionId, respectively).

@dcroote dcroote merged commit ee3e4cd into master Nov 21, 2022
@dcroote dcroote deleted the dcroote/issue1351 branch November 21, 2022 15:13
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.

Add more extensive logging for gateway requests
2 participants