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

Add logging of web-config-server requests to the api_request_log table #4049

Merged
merged 2 commits into from
Jul 28, 2022

Conversation

rohan-bes
Copy link
Collaborator

Issue https://beyondessential.slack.com/archives/C02HES8HFQF/p1658966488670609:

Just added logging of api requests to the web-config-server app.

@@ -62,6 +60,7 @@ export async function createApp() {

// API router
app.use('/api/v1', getRoutesForApiV1());
app.use('/api/v1', logApiRequest(modelRegistry, 'tupaia', 1));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strictly speaking I suppose this should be called web-config rather than tupaia. However, I think that in an ideal world this server would be called tupaia-server, and I wouldn't mind kickstarting that transition. Also, since non-devs may be viewing these logs, having a user friendly name seems apt. Happy to hear reviewer's thoughts though 👍

Copy link
Member

@edmofro edmofro left a comment

Choose a reason for hiding this comment

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

lgtm, just wonder if we need to repeat that util code

* Copyright (c) 2017 - 2022 Beyond Essential Systems Pty Ltd
*/

export const logApiRequest = (models, apiName, version) => async (req, res, next) => {
Copy link
Member

Choose a reason for hiding this comment

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

Would the util exported from server-boilerplate work here instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically the way the userId is determined is different. However if I'm being honest I was a bit worried about creating a dependency on server-boilerplate. Now that I check, there's already a dependency there! 🤦

I thiiiink it's might still be cleaner to do it separate rather than try and make the userId logic more generic... hopefully we switch this bad boy over to server-boilerplate sometime in the not too distant future and then we can scrap this?

Copy link
Member

Choose a reason for hiding this comment

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

Sweet sounds good - so long as you'd considered it!

@rohan-bes rohan-bes merged commit 676f375 into dev Jul 28, 2022
@rohan-bes rohan-bes deleted the api-request-log-wcs branch July 28, 2022 05:47
@rohan-bes rohan-bes mentioned this pull request Jul 28, 2022
3 tasks
@chris-pollard chris-pollard mentioned this pull request Aug 8, 2022
3 tasks
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.

2 participants