You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Spike - Logging consolidation - Do our trace decorators bring any value? #9724
See if the decorators have value wherever they are used or if we can replace them with the exported trace functions, and revisit/dev design the useful ones (did I just create a spike from a spike).
changed the title [-]Logging consolidation - Check the value of our trace decorators[/-][+]Logging consolidation - Do our trace decorators bring any value?[/+]on Jan 22, 2020
changed the title [-]Logging consolidation - Do our trace decorators bring any value?[/-][+]Spike - Logging consolidation - Do our trace decorators bring any value?[/+]on Jan 27, 2020
The only decorator which seems to be useful is @traceDecorators.error. To use traceError you've to explicitly put the try..catch block around it, whereas having a decorator makes it much easier to maintain.
@traceDecorators.verbose() is the major culprit in spamming the Developer console as it also prints the return value object, arguments, and time taken to execute the method.
The time taken to execute the method can be of little use. For eg. to figure out if fetching conda interpreters takes most of the time. But we use CPU profiles to figure those out.
So IMO we should be getting rid of all instances of @traceDecorators.verbose/info/warn().
For the traces provided by @traceDecorators.verbose that could be useful, IMO we should keep using the decorators.
We can decide which traces are useful when reviewing the PR, which will include the DS team.
we should be getting rid of @traceDecorators.verbose/info/warn()
I don't think we should remove all. We (DS team) use a lot of these trace messages when diagnosing bugs & perf related issues.
We check what interpreters have been found, discovered, whether ds server has started, etc.
For eg. to figure out if fetching conda interpreters takes most of the time during activation
Not sure this is true. Extension loads even if conda interpreters takes a long time to load. Karthik Nadig (@karthiknadig) I think we had a chat about this. Finding interpreters is a background task, E.g. extension loads in < 180ms for me, finding conda envs takes ~ 1 second for me on a Mac.
That maybe true but that's not the point I was making.
As far as from the core extension side, these perf traces have not really been helpful, we use CPU profiles to analyse perf issues in bug reports. I agree that maybe we should not remove traces which are common between the extension and DS and which can help you guys.
Don Jayamanne (@DonJayamanne) This spike is only looking at if we need tracing decorators. I agree, we had a chat about interpreter info being a background task and does not directly affect load time. This is only about the decorator them selves and if whatever tracing is captured there is useful.
selves and if whatever tracing is captured there is useful.
The logging about what interpreter information has been found and list of interpeters (big blog of json) is not useful.
It was useful when the code was initially written. I don't think anyone has looked at that except myself when it was originally written.
Removing that alone will clean up the output significantly
Activity
[-]Logging consolidation - Check the value of our trace decorators[/-][+]Logging consolidation - Do our trace decorators bring any value?[/+][-]Logging consolidation - Do our trace decorators bring any value?[/-][+]Spike - Logging consolidation - Do our trace decorators bring any value?[/+]karrtikr commentedon Jan 29, 2020
The only decorator which seems to be useful is
@traceDecorators.error. To usetraceErroryou've to explicitly put thetry..catchblock around it, whereas having a decorator makes it much easier to maintain.@traceDecorators.verbose()is the major culprit in spamming the Developer console as it also prints the return value object, arguments, and time taken to execute the method.The time taken to execute the method can be of little use. For eg. to figure out if fetching conda interpreters takes most of the time. But we use CPU profiles to figure those out.
So IMO we should be getting rid of all instances of@traceDecorators.verbose/info/warn().For the traces provided by
@traceDecorators.verbosethat could be useful, IMO we should keep using the decorators.We can decide which traces are useful when reviewing the PR, which will include the DS team.
DonJayamanne commentedon Jan 29, 2020
I don't think we should remove all. We (
DSteam) use a lot of thesetracemessages when diagnosing bugs & perf related issues.We check what interpreters have been found, discovered, whether ds server has started, etc.
Rich Chiodo (@rchiodo) /cc
DonJayamanne commentedon Jan 29, 2020
Not sure this is true. Extension loads even if conda interpreters takes a long time to load.
Karthik Nadig (@karthiknadig) I think we had a chat about this. Finding interpreters is a background task, E.g. extension loads in < 180ms for me, finding conda envs takes ~ 1 second for me on a Mac.
karrtikr commentedon Jan 29, 2020
That maybe true but that's not the point I was making.
As far as from the core extension side, these perf traces have not really been helpful, we use CPU profiles to analyse perf issues in bug reports. I agree that maybe we should not remove traces which are common between the extension and DS and which can help you guys.
karthiknadig commentedon Jan 29, 2020
Don Jayamanne (@DonJayamanne) This spike is only looking at if we need tracing decorators. I agree, we had a chat about interpreter info being a background task and does not directly affect load time. This is only about the decorator them selves and if whatever tracing is captured there is useful.
DonJayamanne commentedon Jan 30, 2020
The logging about what interpreter information has been found and list of interpeters (big blog of json) is not useful.
It was useful when the code was initially written. I don't think anyone has looked at that except myself when it was originally written.
Removing that alone will clean up the output significantly
karrtikr commentedon Jan 30, 2020
Don Jayamanne (@DonJayamanne) Which verbose do you use to track these then? If you don't need the actually interpreter info.
As I noted in my original comment #9724 (comment), we can decide which verbose are important while reviewing the PRs.
karrtikr commentedon Jan 30, 2020
Closed in favor of #9837