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.

Sign up for GitHub

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

Jump to bottom

[sitecore-jss][Next.js] Introduce performance metrics for debug logging #1555

Merged
merged 3 commits into from
Jul 13, 2023
Merged

[sitecore-jss][Next.js] Introduce performance metrics for debug logging #1555

merged 3 commits into from
Jul 13, 2023

Conversation

illiakovalenko
Copy link
Contributor

@illiakovalenko illiakovalenko commented Jul 10, 2023 •
edited
Loading

Description / Motivation

  • Introduced common debug namespace
  • Added timestamp for base services, nextjs middlewares, page-props-factory

Testing Details

  • Unit Test Added
  • Manual Test/Other - verified that timestamp is present in debug log

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@illiakovalenko illiakovalenko requested a review from a team July 10, 2023 09:34
addy-pathania
addy-pathania approved these changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@addy-pathania addy-pathania left a comment

Choose a reason for hiding this comment

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

Looks great 💯

ambrauer
ambrauer reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

See one comment. Also, wonder if we should retire the http namespace (our fallback for all 3 request clients) and use common instead?

packages/create-sitecore-jss/src/templates/nextjs/src/lib/middleware/index.ts Show resolved Hide resolved
@illiakovalenko
Copy link
Contributor Author

@ambrauer I think it's possible, I was struggling between http and common (wanted to use http for middleware)
I will replace http by common

@illiakovalenko
Copy link
Contributor Author

@ambrauer and I agreed to leave it as it is, better to keep a separation between HTTP logs and common/utils logs that can be added to the sample app

@illiakovalenko illiakovalenko merged commit a3668a9 into dev Jul 13, 2023
@illiakovalenko illiakovalenko deleted the feature/589611 branch July 13, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambrauer ambrauer ambrauer left review comments

@addy-pathania addy-pathania addy-pathania approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@illiakovalenko @ambrauer @addy-pathania