This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Reduce api token logging #1823
Merged
Reduce api token logging #1823
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1823 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: +5.71 kB (+1%) Total Size: 798 kB
ℹ️ View Unchanged
|
dhruvkb
reviewed
Sep 18, 2022
There was a problem hiding this comment.
Given that the implementation currently works, I'm ok with removing the logging.
But would it be a better solution to make the log function a no-op on the production server-side? That allows us to keep the detailed logging in dev untouched, just in case.
openverse-frontend/src/utils/console.ts
Lines 3 to 8 in bb12c58
|
@dhruvkb I'm not sure; they also clutter local development considerably too. Perhaps I should comment them out rather than delete for easy restoration if there's ever issues with the token implementation? |
dhruvkb
approved these changes
Sep 19, 2022
obulat
approved these changes
Sep 20, 2022
| @@ -176,12 +161,6 @@ const apiToken: Plugin = async (context, inject) => { | |||
| let openverseApiToken: string | undefined | |||
| try { | |||
| openverseApiToken = await getApiAccessToken(context) | |||
There was a problem hiding this comment.
Maybe we should leave these lines so that it's clear if we are using the token when fetching or not?
There was a problem hiding this comment.
I'm not sure. We're already sending the error('Unable to retrieve API token, clearing existing token', e) when things fail, which i think would be enough information to debug this token code in the event of a problem.
|
@sarayourfriend as the original author would you mind looking at this before I merge? |
|
What was the increase in log storage costs? We had zero logs before on the frontend so I'm not surprised if it went up several thousand percent, but I would like to know what the overall cost of these logs is. Can we remove the logs that happen on every request but keep the ones around the token refresh? Those should only happen every 12 hours per instance. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We've had a pretty large increase to our log storage costs in production. This PR tries to remove some of the logs from the API token process. I'm opening to keeping a subset of these or whatever the minimum amount of information we need to debug problems with this is.
Testing Instructions
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin