DR-3325: Cleaning up logging and README #264

7emansell · 2024-12-19T22:15:36Z

Ticket:

JIRA ticket DR-3325

This PR does the following:

Resolves errors that were cluttering our logs (specifically the location is not defined from the home page and the lack of unique IDs on the skeleton loaders) and unused console.logs
Starts using the logger (which was set up ages ago) for errors in fetchAPI
- Adds a setImmediate mock to our Jest setup so the logger works in the test environment
Updates the README extensively (deleting unused/redundant sections, adding a table of contents, and adding a section for our logging standards for both server and client side)

Open Questions

Our logging is still split between old and new DC (at least as far as I can tell on QA Cloudwatch and New Relic), which is a little odd, but I guess won't be a problem once Facelift is complete. Do you guys think this should be/can be addressed now?

How has this been tested? How should a reviewer test this?

This branch is currently hooked up to QA so you can see New Relic errors and logs here. In this process, I've found that New Relic collects logs and errors for an application in several places, which can be confusing, so see the updated README logging section for more detail on what you should expect to see.

Winston logs should appear in the Cloudwatch dc-frontend-qa log group.

If you can't be bothered to do all this... this is what they look like :)

Cloudwatch:

New Relic error:

New Relic log:

I will revert the GHA to disconnect this branch from QA once you guys check it out.

You can also run the tests to see the format of server side error logs from fetchAPI, and check the console and Cloudwatch log streams to notice how clean and empty they are.

Also, please check out the README since I got a little carried away and there's more changes than just the addition of the logging section.

Accessibility concerns or updates

Checklist:

I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.
I have updated the CHANGELOG.md.

vercel · 2024-12-19T22:15:41Z

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

Name	Status	Preview	Comments	Updated (UTC)
digital-collections	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 30, 2024 5:09pm

7emansell · 2024-12-20T16:04:47Z

README.md

+| Branch     | Environment | AWS Account    | Cluster                                       | Link To Application                                                                    |
+| :--------- | :---------- | :------------- | :-------------------------------------------- | :------------------------------------------------------------------------------------- |
+| main       | development |                |                                               | [localhost:3000](http://localhost:3000)                                                |
+| qa         | qa          | nypl-dams-dev  | nyplorg-dc-rp-qa AND dc-frontend-qa           | [https://qa-digitalcollections.nypl.org/](https://qa-new-digitalcollections.nypl.org/) |


Is this confusing to list two clusters/ is this technically correct? I think the fact that I'm STILL unsure about this setup means we need to have a very clear written description so I'd like to make that happen

That's fair and I can include that later, but no, this is not technically correct. The cluster is only dc-frontend-qa (similar for prod). The reverse proxy cluster is its own thing.

EdwinGuzman · 2024-12-20T19:47:06Z

Our logging is still split between old and new DC (at least as far as I can tell on QA Cloudwatch and New Relic), which is a little odd, but I guess won't be a problem once Facelift is complete. Do you guys think this should be/can be addressed now?

This isn't great but if we can live with this for now, let's keep it this way. It might be more effort to consolidate both apps, on Cloudwatch and NR, than to just wait until DCF completely replaces legacy.

EdwinGuzman · 2024-12-20T20:04:49Z

If you can't be bothered... this is what they look like

Ha, thanks, but I did look. Did you cause the fetchAPI call to error multiple times? It's great to see this formatted properly in the logs but I'm curious why it's logging so often.

7emansell · 2024-12-20T20:13:56Z

Ha, thanks, but I did look. Did you cause the fetchAPI call to error multiple times? It's great to see this formatted properly in the logs but I'm curious why it's logging so often.

I think you're talking about Cloudwatch logs from today/yesterday? I did try it a bunch because I initially expected the logs to go to the QA RP log group. It should not still be logging unless fetchAPI actually errors. This is getting difficult to look at because Alessandra just deployed to QA from the normal branch, so there's some time periods that don't reflect the logging updates here @EdwinGuzman

app/src/components/pages/aboutPage/aboutPage.tsx

EdwinGuzman · 2024-12-20T20:06:43Z

app/src/components/pages/divisionPage/divisionPage.tsx

@@ -106,7 +106,7 @@ export default function DivisionPage({ data }: any) {
          laneName={data.name}
        />
      ) : (
-        <LaneLoading withTitle={false} />
+        <LaneLoading id={"unloaded"} withTitle={false} />


nit: you don't need braces around string prop values

EdwinGuzman · 2024-12-20T20:15:21Z

app/src/components/pages/divisionsPage/divisionsPage.tsx

@@ -19,6 +19,11 @@ interface DivisionsProps {

 export default function DivisionsPage({ summary, divisions }: DivisionsProps) {
  const [isLoaded, setIsLoaded] = useState(false);
+  if ((window as any).newrelic) {
+    (window as any).newrelic.log("test log from divisions page", {


Do you know where I can see this in NR? I see logs here https://one.newrelic.com/logger?account=121334&duration=86400000&filters=%28domain%20IN%20%28%27BROWSER%27%2C%20%27BROWSER%27%29%20AND%20type%20IN%20%28%27APPLICATION%27%2C%20%27MICRO_FRONTEND%27%29%29&state=97ecb909-d652-d0ee-7371-53a1dcc37c0e

but it's only the "test error from fetch API". I was confused, I saw the following in NR but not in the repo and then noticed from the commits that you added and then removed it. From what I understand, you logged "test error from fetch API" through the logger which uses winston and that did log to cloudwatch. I was not expecting that to log to NR unless if I missed a commit.

Oh interesting ! I wasn't expecting it to log to NR either, since I was looking at the browser logs. But NR is actually picking up everything I guess via this other APM service– so all those "test error from fetch API" were just logging whenever fetchAPI was called at all, so that makes sense.

Testing with the divisions page now...

EdwinGuzman

Some comments in the readme. I appreciate taking the time to tidy it and def not carried away.

EdwinGuzman · 2024-12-20T20:21:24Z

README.md

+| Branch     | Environment | AWS Account    | Cluster                                       | Link To Application                                                                    |
+| :--------- | :---------- | :------------- | :-------------------------------------------- | :------------------------------------------------------------------------------------- |
+| main       | development |                |                                               | [localhost:3000](http://localhost:3000)                                                |
+| qa         | qa          | nypl-dams-dev  | nyplorg-dc-rp-qa AND dc-frontend-qa           | [https://qa-digitalcollections.nypl.org/](https://qa-new-digitalcollections.nypl.org/) |


That's fair and I can include that later, but no, this is not technically correct. The cluster is only dc-frontend-qa (similar for prod). The reverse proxy cluster is its own thing.

EdwinGuzman · 2024-12-20T20:22:38Z

README.md


-### Release Naming Strategy
+### Release naming strategy


I don't have a strong opinion but why the sentence case? UX patterns is to use sentence case for titles and I'm cool with this being a convention we follow on our docs.

I just prefer it and the casing was inconsistent, so i wanted to pick one

EdwinGuzman · 2024-12-20T20:26:28Z

README.md

+
+We structure our logs according to these [NYPL standards](https://github.com/NYPL/engineering-general/blob/main/standards/logging.md). See `logger.js` for the formatting and storage of logs.
+
+Do NOT use `console.log` if you want to add a permanent log, and make sure to clean up your debugging `console.log`s, because they will clutter Cloudwatch and New Relic.


I like the language used, "NOT", "DO". Don't change it, but I wonder if there's a way to use what's in engineering-general or perhaps we should extend engineering-general when it says

The key words MUST, MUST NOT, SHOULD, SHOULD NOT, and MAY/OPTIONAL, in these documents are to be interpreted as described in RFC 2119.

7emansell · 2024-12-20T22:29:38Z

About to be off for the week so just pushed Alessandra's last PR up to QA again, will return to this 12/30!

7emansell · 2024-12-30T17:30:59Z

@EdwinGuzman No significant code changes from your last review, just experimented with what New Relic actually captures depending on how you send it (noticeError or log) and then updated README accordingly

7emansell added 8 commits December 16, 2024 13:23

Removing router.replace on the homepage

eb1c320

Adding keys to listed elements

7d3fb60

Nvm about the lane keys

bcdbec6

Updating laneloading to get rid of key error

0511073

Logger in fetchApi

252e57f

Got a little carried away on the readme

b1146e3

Test logs (new relic and winston)

899f851

Updating git qa workflow so I can test

cd8decc

7emansell added the WIP Work in progress (don't merge) label Dec 19, 2024

vercel bot deployed to Preview December 19, 2024 22:16 View deployment

Momentarily removing some stuff from the qa deploy file

e130686

vercel bot deployed to Preview December 19, 2024 22:22 View deployment

Cleaning up jest setup/mocking

994902d

vercel bot deployed to Preview December 20, 2024 15:05 View deployment

Adding new relic log to the test error page

00ef8ad

vercel bot deployed to Preview December 20, 2024 15:08 View deployment

7emansell requested review from EdwinGuzman and avertrees December 20, 2024 15:47

7emansell removed the WIP Work in progress (don't merge) label Dec 20, 2024

Typos

580b186

vercel bot deployed to Preview December 20, 2024 16:02 View deployment

7emansell commented Dec 20, 2024

View reviewed changes

EdwinGuzman reviewed Dec 20, 2024

View reviewed changes

Testing Nr log levels

7f0c38b

vercel bot deployed to Preview December 20, 2024 20:27 View deployment

EdwinGuzman reviewed Dec 20, 2024

View reviewed changes

Trying level error

215f4c7

vercel bot deployed to Preview December 20, 2024 20:39 View deployment

Testing more stuff

e4e6c15

vercel bot deployed to Preview December 20, 2024 20:47 View deployment

noticeError is the only thing working

e8e110c

vercel bot deployed to Preview December 20, 2024 20:53 View deployment

Testing more log configurations

96470d0

vercel bot deployed to Preview December 30, 2024 15:16 View deployment

Adding errors back in (notice error)

c3df5c0

vercel bot deployed to Preview December 30, 2024 15:37 View deployment

notice error inside useEffect

528ee38

vercel bot deployed to Preview December 30, 2024 15:45 View deployment

fetchApi again

fa63016

vercel bot deployed to Preview December 30, 2024 15:59 View deployment

Warn not an option, trying info

0d67091

vercel bot deployed to Preview December 30, 2024 16:04 View deployment

Found where the logs re going

15d7272

vercel bot deployed to Preview December 30, 2024 16:16 View deployment

Clean up

79bf093

vercel bot deployed to Preview December 30, 2024 16:25 View deployment

Updating readme and testing a console log

0b3bb7b

vercel bot deployed to Preview December 30, 2024 16:48 View deployment

More experimentation

71d3fde

vercel bot deployed to Preview December 30, 2024 16:57 View deployment

7emansell added 2 commits December 30, 2024 12:02

Okay final clean up

b17c5a8

Readme

dd8a0d6

vercel bot deployed to Preview December 30, 2024 17:09 View deployment

7emansell requested a review from EdwinGuzman December 30, 2024 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DR-3325: Cleaning up logging and README #264

DR-3325: Cleaning up logging and README #264

7emansell commented Dec 19, 2024 •

edited

Loading

vercel bot commented Dec 19, 2024 •

edited

Loading

7emansell Dec 20, 2024

EdwinGuzman Dec 20, 2024

EdwinGuzman commented Dec 20, 2024

EdwinGuzman commented Dec 20, 2024

7emansell commented Dec 20, 2024 •

edited

Loading

EdwinGuzman Dec 20, 2024

EdwinGuzman Dec 20, 2024

7emansell Dec 20, 2024 •

edited

Loading

7emansell Dec 20, 2024

EdwinGuzman left a comment

EdwinGuzman Dec 20, 2024

EdwinGuzman Dec 20, 2024

7emansell Dec 20, 2024

EdwinGuzman Dec 20, 2024

7emansell commented Dec 20, 2024

7emansell commented Dec 30, 2024


		We structure our logs according to these [NYPL standards](https://github.com/NYPL/engineering-general/blob/main/standards/logging.md). See `logger.js` for the formatting and storage of logs.

		Do NOT use `console.log` if you want to add a permanent log, and make sure to clean up your debugging `console.log`s, because they will clutter Cloudwatch and New Relic.

DR-3325: Cleaning up logging and README #264

Are you sure you want to change the base?

DR-3325: Cleaning up logging and README #264

Conversation

7emansell commented Dec 19, 2024 • edited Loading

Ticket:

This PR does the following:

Open Questions

How has this been tested? How should a reviewer test this?

Accessibility concerns or updates

Checklist:

vercel bot commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman commented Dec 20, 2024

EdwinGuzman commented Dec 20, 2024

7emansell commented Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7emansell Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

7emansell commented Dec 20, 2024

7emansell commented Dec 30, 2024

7emansell commented Dec 19, 2024 •

edited

Loading

vercel bot commented Dec 19, 2024 •

edited

Loading

7emansell commented Dec 20, 2024 •

edited

Loading

7emansell Dec 20, 2024 •

edited

Loading