Page MenuHomeSoftware Heritage
Differential D7726

loader.core: Add statsd timing metrics
ClosedPublic
Actions

Authored by vlorentz on May 2 2022, 3:21 PM.

Details

Reviewers
anlambert
olasd
Group Reviewers
Reviewers
Maniphest Tasks
T4219: Investigate why GitHub fork detection did not bring a speed-up
Commits
rDLDBASE6ca6d5cf9cef: loader.core: Add statsd timing metrics
Summary

Will give some insights into what is taking the most time

Test Plan

Not tested

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.May 2 2022, 3:21 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMay 2 2022, 3:21 PM
swh-public-ci added a comment.May 2 2022, 3:22 PM

Build has FAILED

Patch application report for D7726 (id=27936)

Rebasing onto 82e1bfad5c...

Current branch diff-target is up to date.
Changes applied before test
commit 619f8aef0fe93725a254b8978220b328b559a3a3
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon May 2 15:20:52 2022 +0200

    loader.core: Add statsd timing metrics

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/764/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/764/console

Harbormaster returned this revision to the author for changes because remote builds failed.May 2 2022, 3:22 PM
Harbormaster failed remote builds in B29007: Diff 27936!
vlorentz edited the summary of this revision. (Show Details)May 2 2022, 3:23 PM
vlorentz added a child revision: D7727: loader.core: Add statsd metrics on collected metadata.
swh-public-ci added a comment.May 2 2022, 3:26 PM

Build is green

Patch application report for D7726 (id=27936)

Rebasing onto 82e1bfad5c...

Current branch diff-target is up to date.
Changes applied before test
commit 619f8aef0fe93725a254b8978220b328b559a3a3
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon May 2 15:20:52 2022 +0200

    loader.core: Add statsd timing metrics

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/766/ for more details.

Harbormaster completed remote builds in B29007: Diff 27936.May 2 2022, 3:26 PM
vlorentz requested review of this revision.May 2 2022, 3:26 PM
vlorentz added a task: T4219: Investigate why GitHub fork detection did not bring a speed-up.May 2 2022, 3:29 PM
anlambert added a subscriber: anlambert.May 2 2022, 3:53 PM

I think you should add some tests checking the expected metrics are sent to statsd by mocking the statsd_timed method.
This will prevent possible regressions on that feature if the code of BaseLoader gets modified.

swh/loader/core/loader.py
433–440

success my be True here if we did not enter the except block.

435

same here

vlorentz updated this revision to Diff 27949.May 3 2022, 1:45 PM

add test and fix lots of bugs

swh-public-ci added a comment.May 3 2022, 1:48 PM

Build is green

Patch application report for D7726 (id=27949)

Rebasing onto c06305e78a...

First, rewinding head to replay your work on top of it...
Applying: loader.core: Add statsd timing metrics
Changes applied before test
commit d16c32648030ea5488d1a0d89651fe2d3a62524e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon May 2 15:20:52 2022 +0200

    loader.core: Add statsd timing metrics

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/769/ for more details.

Harbormaster completed remote builds in B29022: Diff 27949.May 3 2022, 1:48 PM
vlorentz marked 2 inline comments as done.May 3 2022, 1:50 PM
anlambert accepted this revision.May 3 2022, 1:57 PM

Looks good to me.

swh/loader/core/tests/test_loader.py
339

You can directly use mocker.call to gain an import statement.

This revision is now accepted and ready to land.May 3 2022, 1:57 PM
olasd added a subscriber: olasd.May 5 2022, 11:17 AM
olasd added inline comments.
swh/loader/core/loader.py
41

Could probably just be swh_loader as it's fully generic.

483–497
  • We want the metric name to finish with the unit to match the prometheus naming conventions (our statsd exporter configuration only translates dots to underscores)
  • I would suggest making the operation name a tag, as it'll be easier to make generic dashboards that way, even when we add new operations.
olasd added inline comments.May 5 2022, 11:27 AM
swh/loader/core/loader.py
377–378

Are you sure the * 1000 is needed? I'm pretty sure our statsd client does this conversion already and expects seconds as arguments.

vlorentz added inline comments.May 5 2022, 11:43 AM
swh/loader/core/loader.py
377–378

It's done in TimedContextManagerDecorator, which is used by statsd_timed (a decoractor and context manager), not by statsd_timing (a one-off function).

You can see in the test that in the end, everything is multiplied by 1000.

483–497

good point, will do.

vlorentz updated this revision to Diff 28000.May 5 2022, 11:51 AM

rename metrics as suggested

swh-public-ci added a comment.May 5 2022, 11:56 AM

Build is green

Patch application report for D7726 (id=28000)

Rebasing onto a097a946c2...

First, rewinding head to replay your work on top of it...
Applying: loader.core: Add statsd timing metrics
Changes applied before test
commit 34985af30d02575639937d89a444dc0fd91bd0d1
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon May 2 15:20:52 2022 +0200

    loader.core: Add statsd timing metrics

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/774/ for more details.

Harbormaster completed remote builds in B29078: Diff 28000.May 5 2022, 11:56 AM
olasd accepted this revision.May 5 2022, 12:18 PM
olasd added inline comments.
swh/loader/core/loader.py
377–378

Barf. Okay.

swh/loader/core/tests/test_loader.py
340

Hmpf. We should probably use either dots or underscores consistently (not that it really matters much, it's all getting underscored in the prometheus exporter).

vlorentz updated this revision to Diff 28003.May 5 2022, 1:23 PM

consistently use underscores

swh-public-ci added a comment.May 5 2022, 1:26 PM

Build is green

Patch application report for D7726 (id=28003)

Rebasing onto a097a946c2...

First, rewinding head to replay your work on top of it...
Applying: loader.core: Add statsd timing metrics
Changes applied before test
commit 9682fb6fceac4a7e18aa219e0659900f238ee23b
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon May 2 15:20:52 2022 +0200

    loader.core: Add statsd timing metrics

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/776/ for more details.

Harbormaster completed remote builds in B29083: Diff 28003.May 5 2022, 1:26 PM
olasd added a comment.May 5 2022, 1:56 PM

Very nice, thanks!

After reviewing D7732, I'm getting the feeling that it may be worth adding a swh.core.statsd.Statsd instance as attribute of the loader instance, with a set namespace and a set of constant tags that we would not need to repeat for each statsd call (for instance, the visit_type).

This would also allow the loader to collect new constant tags over its lifetime. In this specific case, you could add the success/status tags as soon as they are known. And the git loader could have the "incremental mode" information introduced by D7732 automatically applied to further calls to statsd within the lifetime of the loader, allowing us to get distinct histograms for every use case.

What do you think?

vlorentz added a comment.May 5 2022, 1:59 PM

Good idea, I'll send another diff (based on D7727) to do this

swh/loader/core/loader.py
483–497

Is it fine to call it _seconds even though it's in millisecond; or should I change the suffix to _ms?

olasd added inline comments.May 5 2022, 2:05 PM
swh/loader/core/loader.py
483–497

The prometheus statsd exporter converts it to sensible units, so _seconds is correct

olasd added a comment.May 5 2022, 2:06 PM
In D7726#201823, @vlorentz wrote:

Good idea, I'll send another diff (based on D7727) to do this

Cool, thanks

olasd mentioned this in D7732: Add statsd metrics on incremental loading.May 5 2022, 2:18 PM
vlorentz mentioned this in D7745: Create a new statsd instance to manage namespace and constant tags..May 5 2022, 2:23 PM
vlorentz updated this revision to Diff 28037.May 6 2022, 10:26 AM

rebase

swh-public-ci added a comment.May 6 2022, 10:30 AM

Build is green

Patch application report for D7726 (id=28037)

Rebasing onto a097a946c2...

Current branch diff-target is up to date.
Changes applied before test
commit 6ca6d5cf9cefb595ee8f1ad3fcf5decf4008aafb
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon May 2 15:20:52 2022 +0200

    loader.core: Add statsd timing metrics

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/782/ for more details.

Harbormaster completed remote builds in B29108: Diff 28037.May 6 2022, 10:30 AM
Closed by commit rDLDBASE6ca6d5cf9cef: loader.core: Add statsd timing metrics (authored by vlorentz). · Explain WhyMay 6 2022, 10:35 AM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDLDBASE6ca6d5cf9cef: loader.core: Add statsd timing metrics.

Revision Contents
Changeset List

PathSize
swh/
loader/
core/
68 lines
tests/
77 lines

Diff 28040

View Options

swh/loader/core/loader.py

Loading...
View Options

swh/loader/core/tests/test_loader.py

Loading...
About · Developer information · Legal