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

fix(fxa-admin-panel) temporarily add logging to debug on staging #12749

Merged
merged 1 commit into from
May 4, 2022

Conversation

millmason
Copy link
Contributor

@millmason millmason commented May 3, 2022

Because

  • We're trying to get more insight into errors related to the headers coming into the admin panel in stage.

This pull request

  • Adds logging for the headers and then adds mock logger to tests so that they will still pass.

Issue that this pull request solves

Closes: # n/a

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

@millmason millmason requested a review from a team as a code owner May 3, 2022 21:44
@millmason millmason requested a review from dschom May 3, 2022 21:44
@millmason millmason force-pushed the testing-headers-for-stage branch 2 times, most recently from 7b88171 to a9f86cd Compare May 3, 2022 23:26
Copy link
Contributor

@dschom dschom left a comment

Choose a reason for hiding this comment

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

Looks good! If you can just adjust the logs in client-config/index.ts a tiny bit I think it's good to go!

@@ -126,6 +127,7 @@ export const AccountSearch = () => {

const handleSubmit = (event: React.FormEvent) => {
event.preventDefault();
const trimmedSearchValue = searchInput.trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this from a previous PR or bad merge? Seems trimmedSearchValue is unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was -- removed now!

}

logger.info('userGroupHeader', { 'userGroupHeader.null': userGroupHeader });
logger.info('headers', { headers });
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kinda weird, but these end up as cols in the logs db. Can we just do something like msg: JSON.stringify(headers)? Since this is just a temporary log probably better to just log out a message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just keep these all these as logger.info('userGroupHeader', {...}) It'll make searching for the logs simpler.


@Injectable()
export class UserGroupGuard implements CanActivate {
constructor(private reflector?: Reflector) {}
constructor(private reflector?: Reflector, private log?: MozLoggerService) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@millmason millmason force-pushed the testing-headers-for-stage branch from a9f86cd to 0ad1c74 Compare May 4, 2022 00:13
Copy link
Contributor

@dschom dschom left a comment

Choose a reason for hiding this comment

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

LGTM!

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