Middleware to log necessary data for every request. #3196

Dharshan-K · 2023-11-01T14:21:58Z

This PR is for the Logging Middleware addressed in the issue 2994.

vercel · 2023-11-01T14:22:03Z

@Dharshan-K is attempting to deploy a commit to the Medplum Team on Vercel.

A member of the Team first needs to authorize it.

vercel · 2023-11-01T14:22:05Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
medplum-storybook	⬜️ Ignored (Inspect)	Visit Preview		Nov 4, 2023 11:29pm
medplum-www	⬜️ Ignored (Inspect)	Visit Preview		Nov 4, 2023 11:29pm

sweep-ai · 2023-11-01T14:23:02Z

Apply Sweep Rules to your PR?

Apply: Leftover TODOs in the code should be handled.
Apply: All new business logic should have corresponding unit tests in the tests/ directory.
Apply: Any clearly inefficient or repeated code should be optimized or refactored.

mattwiller

Thanks for the PR @Dharshan-K — I left some comments inline, but this is a great start! Please feel free to try making the requested changes, and let me know if there's anything I can clarify for you

mattwiller · 2023-11-01T17:11:12Z

examples/medplum-chart-demo/dist/index.html

@@ -0,0 +1,14 @@
+<!doctype html>


These files should be in .gitignore and shouldn't be committed; would you mind reverting the changes to files in examples/medplum-chart-demo?

mattwiller · 2023-11-01T17:11:49Z

packages/docs/docusaurus.config.js

@@ -134,7 +134,7 @@ const config = {
                html: `
                <a href="/security"><img src="/img/compliance/soc.png" class="medplum-soc-compliance-image" loading="lazy" alt="SOC"></a>
                <a href="/security"><img src="/img/compliance/hipaa.png" class="medplum-hipaa-compliance-image" loading="lazy" alt="HIPAA"></a>
-                  `,
+                  `,src: 'img/logo.svg',


What was the reason for this change? It seems unrelated to the rest of the PR

mattwiller · 2023-11-01T17:13:24Z

packages/server/package.json

+    "@opentelemetry/api": "^1.4.1",
+    "@opentelemetry/auto-instrumentations-node": "^0.39.2",
+    "@opentelemetry/resources": "^1.17.1",
+    "@opentelemetry/sdk-metrics": "^1.17.0",
+    "@opentelemetry/sdk-node": "^0.43.0",
+    "@opentelemetry/semantic-conventions": "^1.17.1",


I don't think we're ready to start using these dependencies yet, and I didn't see them show up in the rest of the PR. Could you remove these for now? We'll likely tackle publishing metrics via OpenTelemetry in a separate PR.

mattwiller · 2023-11-01T23:06:20Z

packages/server/src/app.ts

+  next();
+  const afterNext = new Date().getTime(); // Record the time after next() completes
+  const totalTime = afterNext - start.getTime(); // Calculate the time taken including the time spent in next()  
+  globalLogger.log('INFO', 'logging data', {requestReceivedTimestamp:start,requestMethodAndPath: `${req.method} ${req.path}`,timeTaken: `${totalTime} ms`, IP_address: req.ip, status: res.statusCode, userAgentString: req.get('User-Agent')});


Instead of using the global logger here, it would be better to use the request-specific one in order to get the request ID and trace ID attached:

const ctx = getRequestContext(); ctx.logger.info(/* ... */);

Additionally, you can access the current user's profile information through the request context:

let userProfile: string | undefined; if (ctx instanceof AuthenticatedRequestContext) { userProfile = ctx.profile.reference; }

A few other minor notes:

INFO is the correct level for this log, but the log message should be more meaningful: consider changing this from "logging data" to something like "Request served" to describe what the log entry is recording

It's a bit hard to scan this long line with all the data properties, could you put each property on its own line?

Since this log is going to be written many times and will likely make up a significant portion of overall log volume, I would like to try making the property names shorter to save space. I know this might seem nitpicky, but tens of bytes can add up fast at scale! I would suggest the following property names: receivedAt, method, path, duration, ip, status, and ua for brevity

Hi @mattwiller, Thank for your feedback. I have changed the code.

Dharshan-K · 2023-11-02T17:50:02Z

Hi @mattwiller, Thank for your feedback. I have changed the code.

mattwiller

This is looking really great, thank you following up @Dharshan-K! It looks like you might have some formatting issues that prettier is complaining about in CI, and I left a couple very minor comments inline: if you could touch those items up we can get this PR merged!

mattwiller · 2023-11-02T18:43:29Z

packages/server/src/app.ts

+  ctx.logger.info('Request served', 
+  {
+    receivedAt:start,
+    requestMethodAndPath: `${req.method}: ${req.path}`,


Could you split this into two different properties: method and path? That will make it easier to query the logs for users:

method: req.method, path: req.path,

mattwiller · 2023-11-02T18:44:43Z

packages/server/src/app.ts

+    Duration: `${totalTime} ms`, 
+    IP: req.ip, status: res.statusCode, 
+    UA: req.get('User-Agent'),
+    Profile: userProfile


These should all have consistent casing:

Suggested change

Duration: `${totalTime} ms`,

IP: req.ip, status: res.statusCode,

UA: req.get('User-Agent'),

Profile: userProfile

durationMs: `${totalTime}`,

ip: req.ip,

status: res.statusCode,

ua: req.get('User-Agent'),

profile: userProfile

Dharshan-K · 2023-11-03T06:42:18Z

Hi @mattwiller, Changes were made to match Prettier workflow. Thank you

coveralls · 2023-11-04T23:42:59Z

coverage: 93.848% (-0.02%) from 93.867%
when pulling 41464d3 on Dharshan-K:logging-data
into e4f361f on medplum:main.

Dharshan-K · 2023-11-05T07:32:04Z

Hi, @codyebberson, the build has failed. what should I do with prettier.

codyebberson · 2023-11-05T15:13:01Z

Hi @Dharshan-K - it looks like an intermittent GitHub Actions error. I'll push it through. No action required. Thank you!

@param

fix(react-native-example): get login working on mobile (#2780) Document "Security Best Practices" for Auth (#3249) Updated Health Gorilla demo to use organization references (#3264) Convert @medplum/react into a module (#3259) Make request logging optional (#3261) Update Health Gorilla demo bot to support multiple Coverages (#3262) Document modeling why a communication was started with reasonCode (#3236) Added example test of patients already being merged (#3260) Added Health Gorilla ABN check (#3257) Added redirects for common search landing pages (#3256) Fix broken links identified by Ahrefs (#3255) Fix `:missing` modifier for token search params (#3218) Add VSCode setting to disable eslint fixAll on save (#3248) fix(fhircast): lower -> mixed, eg. `patient-open` -> `Patient-open` (#3237) Update generator step to run baseschema generator (#3245) Move Coveralls to separate Github Action (#3246) feat(fhircast): add `context.priorVersionId` to `DiagnosticReport-update` (#3232) HMR app on react changes (#3229) Log more details on external auth failure (#3244) Fixes #2703 - document agent system requirements (#3241) Make PKCE optional for external auth providers (#3243) Fixes #2892 - Use Mantine components for field editor (#3234) Added AOE QuestionnaireResponse to Health Gorilla example (#3230) Fixed prettier errors (#3231) Fixes #3215 - external auth fixes for Azure SSO (#3226) Middleware to log necessary data for every request. (#3196) Replace typedoc with api-documenter (#3172) feat(client/fhircast): add `DiagnosticReport-{update,select}` events (#3212) Typo (#3223) Fixes 2890 (#3220) Support custom creation paths (#3214) Suppress warnings from system repo (#3208) Fix "Id" capitolization issue in field selector (#3210) Adjust limit for chained searches (#3206) Fixes #3203 - check references on write with access policy (#3207) Add documentation on resource history (#3182) Fixed api-extractor warnings (#3205) Add placeholder value for identifier references (#3201) Implement search chaining (#3166) Updated Health Gorilla example with Account, Coverage, RelatedPerson (#3202) feat(server/fhircast): add versioning of `DiagnosticReport-open` (#3193) Show display for empty answer selection (#3192) feat(fhircast): add support for `GetCurrentContext` (#3181) Update Next Example project to use App Router (#3197) feat: Pass resource to Repository logEvent on error (#3195) Add documentation for FHIR batch requests (#3175) Fix NPE in reference validation logic (#3186) Add example of how to use optional --base-url flag (#3187) Add to folder structure (#3180) Unique Names for Add Group Button (#3191) Fixed warnings in publish action (#3165) Clarify the Medplum Project Model (#3179) Move to separate file (#3178) Add support for reference type (#3167) Markdown fixes for Docusaurus 3 (#3171) Updating integrations page with refreshed content (#3173) Index medplum-resource search params in example bots (#3164) Add params to MultiSelect for display in modal (#3174) test(client/fhircast): multiple of same key on `diagnosticreport-open` (#3168) Blog Post: Digital Health Operations (#3156) Fixed add-to-project version string (#3162) Add sort-package-json tool (#3161) Adding links from "User Admin Guide" to the '/invite' API (#3158) docs(config): add docs for `external secrets` (#3157) Adding links from "User Admin Guide" to the '/invite' API Refactor QuestionnaireForm (#3109) feat(client/fhircast): add support for `STU3` events (#3143) Health Gorilla diagnosis codes, specimen datetiem, and order notes (#3155) Require hyphen after @param jsdoc (#3154)

@param

fix(react-native-example): get login working on mobile (#2780) Document "Security Best Practices" for Auth (#3249) Updated Health Gorilla demo to use organization references (#3264) Convert @medplum/react into a module (#3259) Make request logging optional (#3261) Update Health Gorilla demo bot to support multiple Coverages (#3262) Document modeling why a communication was started with reasonCode (#3236) Added example test of patients already being merged (#3260) Added Health Gorilla ABN check (#3257) Added redirects for common search landing pages (#3256) Fix broken links identified by Ahrefs (#3255) Fix `:missing` modifier for token search params (#3218) Add VSCode setting to disable eslint fixAll on save (#3248) fix(fhircast): lower -> mixed, eg. `patient-open` -> `Patient-open` (#3237) Update generator step to run baseschema generator (#3245) Move Coveralls to separate Github Action (#3246) feat(fhircast): add `context.priorVersionId` to `DiagnosticReport-update` (#3232) HMR app on react changes (#3229) Log more details on external auth failure (#3244) Fixes #2703 - document agent system requirements (#3241) Make PKCE optional for external auth providers (#3243) Fixes #2892 - Use Mantine components for field editor (#3234) Added AOE QuestionnaireResponse to Health Gorilla example (#3230) Fixed prettier errors (#3231) Fixes #3215 - external auth fixes for Azure SSO (#3226) Middleware to log necessary data for every request. (#3196) Replace typedoc with api-documenter (#3172) feat(client/fhircast): add `DiagnosticReport-{update,select}` events (#3212) Typo (#3223) Fixes 2890 (#3220) Support custom creation paths (#3214) Suppress warnings from system repo (#3208) Fix "Id" capitolization issue in field selector (#3210) Adjust limit for chained searches (#3206) Fixes #3203 - check references on write with access policy (#3207) Add documentation on resource history (#3182) Fixed api-extractor warnings (#3205) Add placeholder value for identifier references (#3201) Implement search chaining (#3166) Updated Health Gorilla example with Account, Coverage, RelatedPerson (#3202) feat(server/fhircast): add versioning of `DiagnosticReport-open` (#3193) Show display for empty answer selection (#3192) feat(fhircast): add support for `GetCurrentContext` (#3181) Update Next Example project to use App Router (#3197) feat: Pass resource to Repository logEvent on error (#3195) Add documentation for FHIR batch requests (#3175) Fix NPE in reference validation logic (#3186) Add example of how to use optional --base-url flag (#3187) Add to folder structure (#3180) Unique Names for Add Group Button (#3191) Fixed warnings in publish action (#3165) Clarify the Medplum Project Model (#3179) Move to separate file (#3178) Add support for reference type (#3167) Markdown fixes for Docusaurus 3 (#3171) Updating integrations page with refreshed content (#3173) Index medplum-resource search params in example bots (#3164) Add params to MultiSelect for display in modal (#3174) test(client/fhircast): multiple of same key on `diagnosticreport-open` (#3168) Blog Post: Digital Health Operations (#3156) Fixed add-to-project version string (#3162) Add sort-package-json tool (#3161) Adding links from "User Admin Guide" to the '/invite' API (#3158) docs(config): add docs for `external secrets` (#3157) Adding links from "User Admin Guide" to the '/invite' API Refactor QuestionnaireForm (#3109) feat(client/fhircast): add support for `STU3` events (#3143) Health Gorilla diagnosis codes, specimen datetiem, and order notes (#3155) Require hyphen after @param jsdoc (#3154)

inital commit.

841aabf

Dharshan-K requested a review from a team as a code owner November 1, 2023 14:21

mattwiller reviewed Nov 1, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into logging-data

f8fa7d7

Dharshan-K force-pushed the logging-data branch from 0b3ba67 to f8fa7d7 Compare November 2, 2023 08:28

vercel bot deployed to Preview – medplum-www November 2, 2023 08:30 View deployment

Logging Middleware Added.

f59f9be

reshmakh added this to the November 30, 2023 milestone Nov 2, 2023

mattwiller reviewed Nov 2, 2023

View reviewed changes

Changed to match Prettier workflow.

41464d3

codyebberson approved these changes Nov 4, 2023

View reviewed changes

codyebberson enabled auto-merge November 5, 2023 16:51

codyebberson disabled auto-merge November 5, 2023 16:51

codyebberson merged commit 47e43bc into medplum:main Nov 5, 2023
9 of 12 checks passed

This was referenced Nov 7, 2023

Make request logging optional #3261

Merged

Release Version 2.1.11 #3273

Merged

dillonstreator mentioned this pull request Nov 14, 2023

fix request logging middleware status code reporting #3316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Middleware to log necessary data for every request. #3196

Middleware to log necessary data for every request. #3196

Dharshan-K commented Nov 1, 2023

vercel bot commented Nov 1, 2023

vercel bot commented Nov 1, 2023 •

edited

Loading

sweep-ai bot commented Nov 1, 2023

mattwiller left a comment

mattwiller Nov 1, 2023

mattwiller Nov 1, 2023

mattwiller Nov 1, 2023

mattwiller Nov 1, 2023

Dharshan-K Nov 2, 2023

Dharshan-K commented Nov 2, 2023

mattwiller left a comment

mattwiller Nov 2, 2023 •

edited

Loading

mattwiller Nov 2, 2023

Dharshan-K commented Nov 3, 2023

coveralls commented Nov 4, 2023

Dharshan-K commented Nov 5, 2023

codyebberson commented Nov 5, 2023

Middleware to log necessary data for every request. #3196

Middleware to log necessary data for every request. #3196

Conversation

Dharshan-K commented Nov 1, 2023

vercel bot commented Nov 1, 2023

vercel bot commented Nov 1, 2023 • edited Loading

sweep-ai bot commented Nov 1, 2023

Apply Sweep Rules to your PR?

mattwiller left a comment

Choose a reason for hiding this comment

mattwiller Nov 1, 2023

Choose a reason for hiding this comment

mattwiller Nov 1, 2023

Choose a reason for hiding this comment

mattwiller Nov 1, 2023

Choose a reason for hiding this comment

mattwiller Nov 1, 2023

Choose a reason for hiding this comment

Dharshan-K Nov 2, 2023

Choose a reason for hiding this comment

Dharshan-K commented Nov 2, 2023

mattwiller left a comment

Choose a reason for hiding this comment

mattwiller Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

mattwiller Nov 2, 2023

Choose a reason for hiding this comment

Dharshan-K commented Nov 3, 2023

coveralls commented Nov 4, 2023

Dharshan-K commented Nov 5, 2023

codyebberson commented Nov 5, 2023

vercel bot commented Nov 1, 2023 •

edited

Loading

mattwiller Nov 2, 2023 •

edited

Loading