-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
feat(core): emit warning when serialization of module is slow #12739
feat(core): emit warning when serialization of module is slow #12739
Conversation
process.emitWarning( | ||
`The module "${opaqueToken.module}" took ${timeSpentInMs.toFixed( | ||
2, | ||
)}ms to serialize, consider reduce the size of the object. More details: https://github.com/nestjs/nest/issues/12738`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can also suggest them to use factory providers over value providers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I'm not sure if the word object here would help the end user. But yeah, this is not a log to the app, it's an internal one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue could be related to the Factory Providers
but can be also some issue on exports
or provide
.
and I'm not sure if the word object here would help the end user. But yeah, this is not a log to the app, it's an internal one
I don't know a better term to describe, maybe consider reduce the size of the module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, regarding the last line "More details: https://github.com/nestjs/nest/issues/12738"
if we want to put a reference maybe I would make a comment above. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tony133 what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I'm certain we've linked to issues from within our docs. If we really don't want to link to the issue directly, then we should make a new section of the FAQ > Common Errors for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think documenting this in the documentation is a great idea too 😉👍, of course you have to figure out how to handle this part in the documentation 😄 , maybe to create a small section inside common errors ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the message to be a little bit more generic and more assertive about the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the Logger is much better, I like it 😉👍
Pull Request Test Coverage Report for Build 52f35b21-f7de-440f-a2ae-b5106b7146d2
💛 - Coveralls |
const opaqueTokenString = this.getStringifiedOpaqueToken(opaqueToken); | ||
const timeSpentInMs = performance.now() - start; | ||
|
||
if (timeSpentInMs > 10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if 10ms is too small
Can you patch this in some real world project just to see if you'll get the warning? I don't have a good nestjs app using v10 :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, I'm not seeing all of the modules here that I would expect. But I got no warnings with 10ms as the check time.
Added a console.log({ timeSpentInMs, opaqueToken })
to see what we're looking at here
{
timeSpentInMs: 0.5800219774246216,
opaqueToken: {
id: 'e16963f9e209181ad0718',
module: 'InternalCoreModule',
dynamic: { providers: [Array], exports: [Array] }
}
}
{
timeSpentInMs: 0.034444987773895264,
opaqueToken: {
id: 'ad071889dc8187858f81b',
module: 'CacheModule',
dynamic: { global: undefined, imports: [Array], providers: [Array] }
}
}
{
timeSpentInMs: 0.023576021194458008,
opaqueToken: {
id: '58f81b9be89c1564788be',
module: 'OgmaModule',
dynamic: { providers: [Array], exports: [Array] }
}
}
{
timeSpentInMs: 0.02220100164413452,
opaqueToken: {
id: '58f81b9be89c1564788be',
module: 'OgmaModule',
dynamic: { providers: [Array], exports: [Array] }
}
}
{
timeSpentInMs: 0.03898298740386963,
opaqueToken: {
id: '226f684678c69bb3e32a1',
module: 'ServerLoggingModule',
dynamic: { imports: [Array], providers: [Array] }
}
}
{
timeSpentInMs: 0.02251499891281128,
opaqueToken: {
id: '84678c69bb3e32a1c97ed',
module: 'OgmaCoreModule',
dynamic: { imports: [Array], providers: [Array] }
}
}
{
timeSpentInMs: 0.020390987396240234,
opaqueToken: {
id: '84678c69bb3e32a1c97ed',
module: 'OgmaCoreModule',
dynamic: { imports: [Array], providers: [Array] }
}
}
{
timeSpentInMs: 0.025695979595184326,
opaqueToken: {
id: '58f81b9be89c1564788be',
module: 'OgmaModule',
dynamic: { providers: [Array], exports: [Array] }
}
}
{
timeSpentInMs: 0.08169996738433838,
opaqueToken: {
id: '226f684678c69bb3e32a1',
module: 'ServerLoggingModule',
dynamic: { imports: [Array], providers: [Array] }
}
}
{
timeSpentInMs: 0.029079020023345947,
opaqueToken: {
id: 'ad071889dc8187858f81b',
module: 'CacheModule',
dynamic: { global: undefined, imports: [Array], providers: [Array] }
}
}
{
timeSpentInMs: 0.022864997386932373,
opaqueToken: {
id: '58f81b9be89c1564788be',
module: 'OgmaModule',
dynamic: { providers: [Array], exports: [Array] }
}
}
{
timeSpentInMs: 0.06157100200653076,
opaqueToken: {
id: '58f81b9be89c1564788be',
module: 'OgmaModule',
dynamic: { providers: [Array], exports: [Array] }
}
}
{
timeSpentInMs: 0.07260900735855103,
opaqueToken: {
id: '84678c69bb3e32a1c97ed',
module: 'OgmaCoreModule',
dynamic: { imports: [Array], providers: [Array] }
}
}
{
timeSpentInMs: 0.04011797904968262,
opaqueToken: {
id: '58f81b9be89c1564788be',
module: 'OgmaModule',
dynamic: { providers: [Array], exports: [Array] }
}
}
Everything looks pretty fast here.
I did force it to error by setting the time to be greater than to 0
and this is what the output looks like
Do we want to emit full warnings or just write logs here about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I have access to Logger
in that part of the code, I prefer to use logger but if we don't have access, emitWarning is the best option for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, as this is all about internal stuff that may not be fixable, I prefer the emitWarning
But we could emit it only if NEST_DEBUG
is enabled. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like to hide it behind a flag because how the people who are having this issue will know about this flag?
I prefer to leave it as directly as possible, and maybe only add a flag to disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will push a update to use Logger
in this case, which I think is way better than emitWarning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
LGTM |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The serialization can be slow and the user will never know about it.
Issue Number: #12738
Fixes #12738
What is the new behavior?
Now we emit a warning using
process.emitWarning
when the serialization took more than10ms
.This is an arbitrary value, we can raise or lower this value depending on the real-world applications.
Does this PR introduce a breaking change?