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

Polish trace back-logging and other logging aspects #6746

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

mih
Copy link
Member

@mih mih commented Jun 4, 2022

Changelog

💫 Enhancements and new features

Documentation

@mih mih added documentation Documentation-related issue semver-minor Increment the minor version when merged labels Jun 4, 2022
@mih mih changed the title Doclog Polish trace back-logging and other logging aspects Jun 4, 2022
mih added 4 commits June 4, 2022 13:30
This change aims to make the output more compact and more
informative.

The stack frame of the log handler re-emitting a message is not longer
included in the report. This is a uniform internal behavior that is
identical for all messages, and not useful for debugging with this
utility (because the previously established removal of internal
logging handling from the traceback already made it unsuitable for
debugging logging code -- which is perfectly fine).

Was
```
% DATALAD_LOG_TRACEBACK=collide dl -l debug status
[DEBUG  ] datalad:33>main:86>parser:119,369>log:223  Command line args 1st pass for DataLad 0.16.2+190.gd657c17fe. Parsed: Namespace() Unparsed: ['status']
[DEBUG  ] ...>main:94>log:223  Parsing known args among ['/home/mih/env/datalad-dev/bin/datalad', '-l', 'debug', 'status']
```

Now
```
% DATALAD_LOG_TRACEBACK=collide dl -l debug status
[DEBUG  ] datalad:33>main:86>parser:119,369  Command line args 1st pass for DataLad 0.16.2+194.g83e04f37f.dirty. Parsed: Namespace() Unparsed: ['status']
[DEBUG  ] …>main:94  Parsing known args among ['/home/mih/env/datalad-dev/bin/datalad', '-l', 'debug', 'status']
```

`DATALAD_LOG_TRACEBACK=1` will now typically report the origin of
any log message, rather than a location in `log.py`, a more natural
behavior IMHO. Due to the previously established removal of "common"
stack frames, the traceback depth already could not be interpreted
in a strict manner.

Was

```
% DATALAD_LOG_TRACEBACK=1 dl -l debug status
[DEBUG  ] ...>log:223  Command line args 1st pass for DataLad 0.16.2+190.gd657c17fe. Parsed: Namespace() Unparsed: ['status']
[DEBUG  ] ...>log:223  Parsing known args among ['/home/mih/env/datalad-dev/bin/datalad', '-l', 'debug', 'status']
```

Now

```
% DATALAD_LOG_TRACEBACK=1 dl -l debug status
[DEBUG  ] parser:369  Command line args 1st pass for DataLad 0.16.2+194.g83e04f37f.dirty. Parsed: Namespace() Unparsed: ['status']
[DEBUG  ] main:94  Parsing known args among ['/home/mih/env/datalad-dev/bin/datalad', '-l', 'debug', 'status']
```

The actual behavior of this switch, and that it is not a proper
config switch, is now documented.

Closes datalad#6744
Main update of related docs is coming via
datalad#6734
@codeclimate
Copy link

codeclimate bot commented Jun 4, 2022

Code Climate has analyzed commit 2faad43 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Security 2

View more on Code Climate.

@codecov
Copy link

codecov bot commented Jun 4, 2022

Codecov Report

Merging #6746 (2faad43) into master (d657c17) will increase coverage by 1.62%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #6746      +/-   ##
==========================================
+ Coverage   88.79%   90.41%   +1.62%     
==========================================
  Files         353      353              
  Lines       45744    45763      +19     
==========================================
+ Hits        40617    41376     +759     
+ Misses       5127     4387     -740     
Impacted Files Coverage Δ
datalad/interface/common_cfg.py 100.00% <ø> (ø)
datalad/log.py 87.93% <90.00%> (+0.09%) ⬆️
datalad/tests/test_log.py 99.30% <100.00%> (ø)
datalad/local/unlock.py 100.00% <0.00%> (ø)
datalad/interface/common_opts.py 100.00% <0.00%> (ø)
datalad/support/tests/test_annexrepo.py 97.87% <0.00%> (+<0.01%) ⬆️
datalad/core/distributed/tests/test_clone.py 97.59% <0.00%> (+0.01%) ⬆️
datalad/local/tests/test_wtf.py 98.80% <0.00%> (+0.02%) ⬆️
datalad/core/distributed/clone.py 91.24% <0.00%> (+0.03%) ⬆️
datalad/support/annexrepo.py 90.37% <0.00%> (+0.08%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d657c17...2faad43. Read the comment docs.

Comment on lines +465 to +466
'This setting is only in effect when given as an '
'environment variable DATALAD_LOG_TRACEBACK. '
Copy link
Member

Choose a reason for hiding this comment

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

do you give me permission to push to your PR adopted version of
0b604b6
and adjust doc here as

Suggested change
'This setting is only in effect when given as an '
'environment variable DATALAD_LOG_TRACEBACK. '
'This setting is in effect if set from command line via `-c` option. '

Copy link
Member

Choose a reason for hiding this comment

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

also would need to add doc record for DATALAD_LOG_VMEM

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not looked into that other commit closely. If you think this PR requires it, go ahead. Otherwise, I think a new PRs is more suitable than piling on.

@bpoldrack
Copy link
Member

Not sure, what's the state of this. Do you intend to push, @yarikoptic ?

Otherwise I'd say let's merge.

@yarikoptic yarikoptic merged commit fcc2e4d into datalad:master Jun 14, 2022
@mih mih mentioned this pull request Jul 1, 2022
40 tasks
@mih mih deleted the doclog branch July 19, 2022 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation-related issue semver-minor Increment the minor version when merged
Projects
None yet
3 participants