Adding performance statistics to borg create

[francoo98]

Issue #2209 Up until now, I have worked on:

• number of files cache hits (b9d713b)
• number of files cache indicated changed files (b9d713b)
• number of files not in files cache at all (b9d713b)
• time spent in chunking (3e7326e)
• time spent in id hashing (c17f6f1)

The changes are pretty short and simple, I don't think a description is necessary. Let me know what you think and if there is anything that needs to be changed.

[ThomasWaldmann]

It would be good to have a test in borg.testsuite.archiver that checks if the new code works correctly. See the other tests you already see there about how this could work.

[francoo98]

Should the time be printed in seconds? Are decimals needed?

It would be good to have a test in borg.testsuite.archiver that checks if the new code works correctly. See the other tests you already see there about how this could work.

I am thinking about creating a test that would create a few archives and then check that the files_stats variable is correct (e.g: if one file was added "A" should be 2), and check that the printing is correct. I don't know how to test time measures.

[ThomasWaldmann]

IIRC borg create --stats already outputs some timing stats, do the new stats in a similar way concerning units and precision.

Testings the timing stats of course can not be done precisely, but maybe you could check it is > 0.0?

[ThomasWaldmann]

BTW, you are working in your master branch.

Usually you should first create a feature branch for the feature you are working on, that works much better in the end.

You can leave it like it is for this time, but remember it next time.

[ThomasWaldmann]

@francoo98 writing tests seems a bit tedious at first, esp. if you are new to it, but long term it can save countless of hours of manual testing (and/or countless of bugs). Check "TDD / test driven development" and "pytest".

pytest is a quite powerful testing framework, makes writing tests easier than other ones (after you got used to it).

[ThomasWaldmann]

Looks like there is some conflict with master branch, please resolve.

(That will also trigger the tests to run again. Seems like the last test run had some unrelated issue.)

[ThomasWaldmann]

the linter is still unhappy, see github checks.

you can run black .and tox -e flake8 locally to detect and fix such stuff early.

[francoo98]

I have some issues that I don't know how to solve.

1. Where should I put the test for the tx_bytes and rx_bytes?
2. test_hashing_time fails because the execution is too fast, how many files should I create in the test to make it slower?
3. The original issue (#2209) tells to add time spent in encryption, but I don't if this means encryption of repository contents (i.e: -e set to something different than none) or encryption of network traffic (because of a RemoteRepository)

[ThomasWaldmann]

Ehrm, that code is wrong. I meant .removesuffix(), not .rstrip():

hashing_time = float(hashing_time.rstrip(" seconds"))

Please fix all similar places in your PR.

[codecov-commenter]

Codecov Report

Merging #6991 (c367845) into master (a74bcb7) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6991      +/-   ##
==========================================
+ Coverage   83.28%   83.34%   +0.05%     
==========================================
  Files          67       67              
  Lines       11460    11483      +23     
  Branches     2076     2078       +2     
==========================================
+ Hits         9545     9571      +26     
+ Misses       1375     1371       -4     
- Partials      540      541       +1     

Impacted Files	Coverage Δ
src/borg/archive.py	83.38% <100.00%> (+0.17%)	:arrow_up:
src/borg/archiver/create_cmd.py	78.84% <100.00%> (+0.41%)	:arrow_up:
src/borg/remote.py	80.94% <0.00%> (+0.15%)	:arrow_up:
src/borg/platform/base.py	81.45% <0.00%> (+1.61%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[francoo98]

BTW, there is a time.sleep(1) in the test you added, is it needed / why?

It is needed because the second file needs newer timestamp than the first file.

[ThomasWaldmann]

@francoo98 thanks for the PR, it's merged now!

[francoo98]

That's great 😁! Are you going to close the issue?