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

update logs #485

Merged
merged 7 commits into from
Mar 6, 2025
Merged

update logs #485

merged 7 commits into from
Mar 6, 2025

Conversation

charmingduchess
Copy link
Contributor

@charmingduchess charmingduchess commented Mar 5, 2025

There are mysterious errors blocking hold requests in QA. Updating the logs for more granularity to investigate. Also added client-level logging to nypl client gets and posts, and sierra client gets, posts and puts. I did most of the updated requests, and they work! However, I would appreciate some scrutiny on my instance method manipulation.

Copy link

vercel bot commented Mar 5, 2025

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

Name Status Preview Comments Updated (UTC)
research-catalog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 6, 2025 7:40pm

Copy link
Member

@nonword nonword left a comment

Choose a reason for hiding this comment

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

Approving on condition that this not go to prod as is, since we don't want to be logging request details there. Note you can theoretically enable debug logging inside the nypldataapiclient via log_level option to the constructor, so you could do:

    const nyplApiClient = new NyplApiClient({
      ...
      log_level: process.env.NYPL_API_LOG_LEVEL || 'error'
    })

And for sierra-client you may be able to use setLogLevel

@charmingduchess
Copy link
Contributor Author

Are you saying that the logs would be too noisy in production? They log to cloudwatch when not running locally or on Vercel.

@charmingduchess
Copy link
Contributor Author

Ok... final form is using homebaked logs and checking env for log level. The nypl data client logs are suuuper noisy, printing entire response bodies. I cut a ticket to make quieter info level logs with request urls, but for now, this will have to do.

@charmingduchess charmingduchess requested a review from nonword March 5, 2025 18:09
const transports = [
new winston.transports.Console({ level: "debug" }),
new winston.transports.Console({ level: process.env.LOG_LEVEL || "error" }),
Copy link
Member

Choose a reason for hiding this comment

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

All logging was debug before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly do not know. Looks like maybe? I have not touched this file before.

logger.js Outdated
@@ -42,21 +42,22 @@ const initializeLogger = () => {
}
return JSON.stringify(logObject)
})

console.log(process.env.LOG_LEVEL || "error")
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

@charmingduchess charmingduchess merged commit f1e624d into main Mar 6, 2025
2 of 3 checks passed
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.

3 participants