Page MenuHomeSoftware Heritage
Differential D3618

Update swh.storage.origin_get calls to latest api change
ClosedPublic
Actions

Authored by ardumont on Jul 24 2020, 6:30 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T645: Type swh-storage endpoints with swh.model objects
Commits
rDWAPPS26622a3425f4: Update swh.storage.origin_get calls to latest api change
Summary

Note that I'm not modifying anything in the swh-web right now.
I'm just making just conversion happens at the right place so everything is fine. [1]

[1] I believe that when we'll be done with typing swh-storage and that we use swh-model objects everywhere.
We can discuss with @anlambert to simplify a bit the "swh-model" used within swh-web (preceding the almost "pervasive" use of our swh.model now), same goes for the archive-data storage used here (preceding the pytest fixtures).
I recall the main swh-model divergence needed here is a way to convert to hex the hashes in bytes and also make the optional value not shown in the main output result.
Like i said, to discuss later ;)

Related to T645

Test Plan

tox

Diff Detail

Repository
rDWAPPS Web applications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13951
Build 21410: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 21409: arc lint + arc unit

Event Timeline

ardumont created this revision.Jul 24 2020, 6:30 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJul 24 2020, 6:30 PM
swh-public-ci added a comment.Jul 24 2020, 6:31 PM

Build has FAILED

Patch application report for D3618 (id=12725)

Rebasing onto fdd3fe73d7...

Current branch diff-target is up to date.
Changes applied before test
commit 2a44d0cc1e7a610af8914a72663ced97c8efb223
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jul 24 18:29:50 2020 +0200

    Update swh.storage.origin_get calls to latest api change
    
    Related to T645

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

Harbormaster failed remote builds in B13951: Diff 12725!Jul 24 2020, 6:31 PM
ardumont added inline comments.Jul 24 2020, 6:34 PM
swh/web/common/service.py
310

Can't use OriginInfo here as mypy complains rightfully about the dict not being one.

331

thus the conversion from_swh here and not from_origin.

ardumont edited the test plan for this revision. (Show Details)Jul 27 2020, 8:39 AM
swh-public-ci added a comment.Jul 27 2020, 8:51 AM

Build is green

Patch application report for D3618 (id=12725)

Rebasing onto fdd3fe73d7...

Current branch diff-target is up to date.
Changes applied before test
commit 2a44d0cc1e7a610af8914a72663ced97c8efb223
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jul 24 18:29:50 2020 +0200

    Update swh.storage.origin_get calls to latest api change
    
    Related to T645

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

Harbormaster completed remote builds in B13951: Diff 12725.Jul 27 2020, 8:51 AM
vlorentz added a subscriber: vlorentz.Jul 27 2020, 10:15 AM
vlorentz added inline comments.
swh/web/common/service.py
310

Could you add a new TypedDict, then?

337

If that's not too much work, I think you should change the argument to just be the URL.

ardumont edited the summary of this revision. (Show Details)Jul 27 2020, 10:23 AM
ardumont added a subscriber: anlambert.
ardumont added inline comments.Jul 27 2020, 10:39 AM
swh/web/common/service.py
310

same as below, unsure of the impacts.
I'd like to keep those minimal right now (updated the description about my reasoning).

337

No idea of the impact.
I'll have a look.

ardumont added inline comments.Jul 27 2020, 12:06 PM
swh/web/common/service.py
337

ok, this one sounds sensible to rework ;)

ardumont added inline comments.Jul 27 2020, 12:14 PM
swh/web/common/service.py
310

Sounds reasonable a change as well ;)

ardumont updated this revision to Diff 12750.Jul 27 2020, 12:15 PM

Adapt according to review. In the end, the changes were reasonable.

ardumont added inline comments.Jul 27 2020, 12:17 PM
swh/web/common/service.py
331

for an older version of the diff.

swh-public-ci added a comment.Jul 27 2020, 12:28 PM

Build is green

Patch application report for D3618 (id=12750)

Rebasing onto fdd3fe73d7...

Current branch diff-target is up to date.
Changes applied before test
commit 26622a3425f45606e8a0d34d472604cd075cb065
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jul 24 18:29:50 2020 +0200

    Update swh.storage.origin_get calls to latest api change
    
    Related to T645

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

Harbormaster completed remote builds in B13980: Diff 12750.Jul 27 2020, 12:28 PM
vlorentz accepted this revision.Jul 27 2020, 12:40 PM
This revision is now accepted and ready to land.Jul 27 2020, 12:40 PM
Closed by commit rDWAPPS26622a3425f4: Update swh.storage.origin_get calls to latest api change (authored by ardumont). · Explain WhyJul 27 2020, 12:45 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDWAPPS26622a3425f4: Update swh.storage.origin_get calls to latest api change.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
2 lines--
swh/
web/
common/
53 lines91%
Loading...
tests/
api/
views/
6 lines99%
Loading...
common/
2 lines94%
Loading...
6 lines90%
Loading...
3 lines100%
Loading...
3 lines96%
Loading...

Diff 12725

View Options

requirements-swh.txt

Show First 20 Lines • Show All 452 Lines • ▼ Show 20 Lines def build_extrinsic_origin_metadata(self) -> List[RawExtrinsicMetadata]:
self.log.debug("lister_not provided, skipping extrinsic origin metadata") self.log.debug("lister_not provided, skipping extrinsic origin metadata")
return [] return []
assert ( assert (
self.lister_instance_name is not None self.lister_instance_name is not None
), "lister_instance_name is None, but lister_name is not" ), "lister_instance_name is None, but lister_name is not"
metadata = [] metadata = []
for cls in get_fetchers_for_lister(self.lister_name):
fetcher_classes = get_fetchers_for_lister(self.lister_name)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.fetchers",
len(fetcher_classes),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

For all these histograms, we will want to change the configuration of the statsd exporter for this metric to generate a histogram with relevant buckets. Our default buckets are only relevant for timings and will be really wasteful for this (which will pretty much always return 0 or 1)

https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/data/common/common.yaml$3124-3151

https://github.com/prometheus/statsd_exporter#statsd-timers-and-distributions

(something like:

mappings:
 - match: whatever this metric name will end up being
   observer_type: histogram
   histogram_options:
     buckets: [0, 1, 2, 3, 4, 5]

(you will want to test that in docker as well)

olasd: For all these histograms, we will want to change the configuration of the statsd exporter for…
)
for cls in fetcher_classes:
metadata_fetcher = cls( metadata_fetcher = cls(
origin=self.origin, origin=self.origin,
lister_name=self.lister_name, lister_name=self.lister_name,
lister_instance_name=self.lister_instance_name, lister_instance_name=self.lister_instance_name,
credentials=self.metadata_fetcher_credentials, credentials=self.metadata_fetcher_credentials,
) )
with self.statsd_timed("fetch_one_metadata"): with self.statsd_timed(
"fetch_one_metadata", tags={"fetcher": cls.FETCHER_NAME}
):
metadata.extend(metadata_fetcher.get_origin_metadata()) metadata.extend(metadata_fetcher.get_origin_metadata())
if self.parent_origins is None: if self.parent_origins is None:
self.parent_origins = metadata_fetcher.get_parent_origins() self.parent_origins = metadata_fetcher.get_parent_origins()
statsd.histogram(
f"{STATSD_PREFIX}.metadata.parent_origins",
len(self.parent_origins),
tags={"fetcher": cls.FETCHER_NAME, "visit_type": self.visit_type},
)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.objects",
len(metadata),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

Do we really want a histogram, or just a running total? (note that the histogram will also generate a running total)

olasd: Do we really want a histogram, or just a running total? (note that the histogram will also…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I don't know; I'm only interested in the average, and an histogram seemed like the only way to do that (ditto above)

vlorentz: I don't know; I'm only interested in the average, and an histogram seemed like the only way to…
olasdUnsubmitted
Done
Inline Actions

If you just want an average, then you just need to have a pair of metrics: one that is incremented for each operation (and will be shared across all metrics), and one that is incremented by the number of objects you want to count the average of. You can then divide these two to get a running average.

The prometheus statsd exporter just generates these series automatically for histograms, so that they can be used with the prometheus histogram functions: there's a <foo>_count series for the number of times a value was measured, and a <foo>_sum series for the sum of all <foo> values mesured, in addition to all the "bucket counts" series (which generate one series for the number of counts lower than a given bucket limit).

Either way, in terms of storage, as long as we pinpoint a small set of relevant buckets, it shouldn't matter too much. If you think that we're never going to need the fine-grained metrics (I probably agree), please use counters directly.

olasd: If you just want an average, then you just need to have a pair of metrics: one that is…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Done. I kept {name} inside the metric instead of tags; because it doesn't make sense to aggregate them (like in the other diff) IMO

vlorentz: Done. I kept `{name}` inside the metric instead of tags; because it doesn't make sense to…
olasdUnsubmitted
Not Done
Inline Actions

Yeah, that's perfect!

olasd: Yeah, that's perfect!
)
return metadata return metadata
@contextlib.contextmanager @contextlib.contextmanager
def statsd_timed(self, name, tags={}): def statsd_timed(self, name, tags={}):
with statsd.timed( with statsd.timed(
f"{STATSD_PREFIX}.duration_seconds.{name}", f"{STATSD_PREFIX}.duration_seconds.{name}",
tags={"visit_type": self.visit_type, **tags}, tags={"visit_type": self.visit_type, **tags},
▲ Show 20 Lines • Show All 85 Lines • Show Last 20 Lines
View Options

swh/web/common/service.py

Show First 20 Lines • Show All 452 Lines • ▼ Show 20 Lines def build_extrinsic_origin_metadata(self) -> List[RawExtrinsicMetadata]:
self.log.debug("lister_not provided, skipping extrinsic origin metadata") self.log.debug("lister_not provided, skipping extrinsic origin metadata")
return [] return []
assert ( assert (
self.lister_instance_name is not None self.lister_instance_name is not None
), "lister_instance_name is None, but lister_name is not" ), "lister_instance_name is None, but lister_name is not"
metadata = [] metadata = []
for cls in get_fetchers_for_lister(self.lister_name):
fetcher_classes = get_fetchers_for_lister(self.lister_name)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.fetchers",
len(fetcher_classes),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

For all these histograms, we will want to change the configuration of the statsd exporter for this metric to generate a histogram with relevant buckets. Our default buckets are only relevant for timings and will be really wasteful for this (which will pretty much always return 0 or 1)

https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/data/common/common.yaml$3124-3151

https://github.com/prometheus/statsd_exporter#statsd-timers-and-distributions

(something like:

mappings:
 - match: whatever this metric name will end up being
   observer_type: histogram
   histogram_options:
     buckets: [0, 1, 2, 3, 4, 5]

(you will want to test that in docker as well)

olasd: For all these histograms, we will want to change the configuration of the statsd exporter for…
)
for cls in fetcher_classes:
metadata_fetcher = cls( metadata_fetcher = cls(
origin=self.origin, origin=self.origin,
lister_name=self.lister_name, lister_name=self.lister_name,
lister_instance_name=self.lister_instance_name, lister_instance_name=self.lister_instance_name,
credentials=self.metadata_fetcher_credentials, credentials=self.metadata_fetcher_credentials,
) )
with self.statsd_timed("fetch_one_metadata"): with self.statsd_timed(
"fetch_one_metadata", tags={"fetcher": cls.FETCHER_NAME}
):
metadata.extend(metadata_fetcher.get_origin_metadata()) metadata.extend(metadata_fetcher.get_origin_metadata())
if self.parent_origins is None: if self.parent_origins is None:
self.parent_origins = metadata_fetcher.get_parent_origins() self.parent_origins = metadata_fetcher.get_parent_origins()
statsd.histogram(
f"{STATSD_PREFIX}.metadata.parent_origins",
len(self.parent_origins),
tags={"fetcher": cls.FETCHER_NAME, "visit_type": self.visit_type},
)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.objects",
len(metadata),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

Do we really want a histogram, or just a running total? (note that the histogram will also generate a running total)

olasd: Do we really want a histogram, or just a running total? (note that the histogram will also…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I don't know; I'm only interested in the average, and an histogram seemed like the only way to do that (ditto above)

vlorentz: I don't know; I'm only interested in the average, and an histogram seemed like the only way to…
olasdUnsubmitted
Done
Inline Actions

If you just want an average, then you just need to have a pair of metrics: one that is incremented for each operation (and will be shared across all metrics), and one that is incremented by the number of objects you want to count the average of. You can then divide these two to get a running average.

The prometheus statsd exporter just generates these series automatically for histograms, so that they can be used with the prometheus histogram functions: there's a <foo>_count series for the number of times a value was measured, and a <foo>_sum series for the sum of all <foo> values mesured, in addition to all the "bucket counts" series (which generate one series for the number of counts lower than a given bucket limit).

Either way, in terms of storage, as long as we pinpoint a small set of relevant buckets, it shouldn't matter too much. If you think that we're never going to need the fine-grained metrics (I probably agree), please use counters directly.

olasd: If you just want an average, then you just need to have a pair of metrics: one that is…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Done. I kept {name} inside the metric instead of tags; because it doesn't make sense to aggregate them (like in the other diff) IMO

vlorentz: Done. I kept `{name}` inside the metric instead of tags; because it doesn't make sense to…
olasdUnsubmitted
Not Done
Inline Actions

Yeah, that's perfect!

olasd: Yeah, that's perfect!
)
return metadata return metadata
@contextlib.contextmanager @contextlib.contextmanager
def statsd_timed(self, name, tags={}): def statsd_timed(self, name, tags={}):
with statsd.timed( with statsd.timed(
f"{STATSD_PREFIX}.duration_seconds.{name}", f"{STATSD_PREFIX}.duration_seconds.{name}",
tags={"visit_type": self.visit_type, **tags}, tags={"visit_type": self.visit_type, **tags},
▲ Show 20 Lines • Show All 85 Lines • Show Last 20 Lines
View Options

swh/web/tests/api/views/test_origin.py

Show First 20 Lines • Show All 452 Lines • ▼ Show 20 Lines def build_extrinsic_origin_metadata(self) -> List[RawExtrinsicMetadata]:
self.log.debug("lister_not provided, skipping extrinsic origin metadata") self.log.debug("lister_not provided, skipping extrinsic origin metadata")
return [] return []
assert ( assert (
self.lister_instance_name is not None self.lister_instance_name is not None
), "lister_instance_name is None, but lister_name is not" ), "lister_instance_name is None, but lister_name is not"
metadata = [] metadata = []
for cls in get_fetchers_for_lister(self.lister_name):
fetcher_classes = get_fetchers_for_lister(self.lister_name)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.fetchers",
len(fetcher_classes),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

For all these histograms, we will want to change the configuration of the statsd exporter for this metric to generate a histogram with relevant buckets. Our default buckets are only relevant for timings and will be really wasteful for this (which will pretty much always return 0 or 1)

https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/data/common/common.yaml$3124-3151

https://github.com/prometheus/statsd_exporter#statsd-timers-and-distributions

(something like:

mappings:
 - match: whatever this metric name will end up being
   observer_type: histogram
   histogram_options:
     buckets: [0, 1, 2, 3, 4, 5]

(you will want to test that in docker as well)

olasd: For all these histograms, we will want to change the configuration of the statsd exporter for…
)
for cls in fetcher_classes:
metadata_fetcher = cls( metadata_fetcher = cls(
origin=self.origin, origin=self.origin,
lister_name=self.lister_name, lister_name=self.lister_name,
lister_instance_name=self.lister_instance_name, lister_instance_name=self.lister_instance_name,
credentials=self.metadata_fetcher_credentials, credentials=self.metadata_fetcher_credentials,
) )
with self.statsd_timed("fetch_one_metadata"): with self.statsd_timed(
"fetch_one_metadata", tags={"fetcher": cls.FETCHER_NAME}
):
metadata.extend(metadata_fetcher.get_origin_metadata()) metadata.extend(metadata_fetcher.get_origin_metadata())
if self.parent_origins is None: if self.parent_origins is None:
self.parent_origins = metadata_fetcher.get_parent_origins() self.parent_origins = metadata_fetcher.get_parent_origins()
statsd.histogram(
f"{STATSD_PREFIX}.metadata.parent_origins",
len(self.parent_origins),
tags={"fetcher": cls.FETCHER_NAME, "visit_type": self.visit_type},
)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.objects",
len(metadata),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

Do we really want a histogram, or just a running total? (note that the histogram will also generate a running total)

olasd: Do we really want a histogram, or just a running total? (note that the histogram will also…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I don't know; I'm only interested in the average, and an histogram seemed like the only way to do that (ditto above)

vlorentz: I don't know; I'm only interested in the average, and an histogram seemed like the only way to…
olasdUnsubmitted
Done
Inline Actions

If you just want an average, then you just need to have a pair of metrics: one that is incremented for each operation (and will be shared across all metrics), and one that is incremented by the number of objects you want to count the average of. You can then divide these two to get a running average.

The prometheus statsd exporter just generates these series automatically for histograms, so that they can be used with the prometheus histogram functions: there's a <foo>_count series for the number of times a value was measured, and a <foo>_sum series for the sum of all <foo> values mesured, in addition to all the "bucket counts" series (which generate one series for the number of counts lower than a given bucket limit).

Either way, in terms of storage, as long as we pinpoint a small set of relevant buckets, it shouldn't matter too much. If you think that we're never going to need the fine-grained metrics (I probably agree), please use counters directly.

olasd: If you just want an average, then you just need to have a pair of metrics: one that is…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Done. I kept {name} inside the metric instead of tags; because it doesn't make sense to aggregate them (like in the other diff) IMO

vlorentz: Done. I kept `{name}` inside the metric instead of tags; because it doesn't make sense to…
olasdUnsubmitted
Not Done
Inline Actions

Yeah, that's perfect!

olasd: Yeah, that's perfect!
)
return metadata return metadata
@contextlib.contextmanager @contextlib.contextmanager
def statsd_timed(self, name, tags={}): def statsd_timed(self, name, tags={}):
with statsd.timed( with statsd.timed(
f"{STATSD_PREFIX}.duration_seconds.{name}", f"{STATSD_PREFIX}.duration_seconds.{name}",
tags={"visit_type": self.visit_type, **tags}, tags={"visit_type": self.visit_type, **tags},
▲ Show 20 Lines • Show All 85 Lines • Show Last 20 Lines
View Options

swh/web/tests/common/test_service.py

Show First 20 Lines • Show All 452 Lines • ▼ Show 20 Lines def build_extrinsic_origin_metadata(self) -> List[RawExtrinsicMetadata]:
self.log.debug("lister_not provided, skipping extrinsic origin metadata") self.log.debug("lister_not provided, skipping extrinsic origin metadata")
return [] return []
assert ( assert (
self.lister_instance_name is not None self.lister_instance_name is not None
), "lister_instance_name is None, but lister_name is not" ), "lister_instance_name is None, but lister_name is not"
metadata = [] metadata = []
for cls in get_fetchers_for_lister(self.lister_name):
fetcher_classes = get_fetchers_for_lister(self.lister_name)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.fetchers",
len(fetcher_classes),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

For all these histograms, we will want to change the configuration of the statsd exporter for this metric to generate a histogram with relevant buckets. Our default buckets are only relevant for timings and will be really wasteful for this (which will pretty much always return 0 or 1)

https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/data/common/common.yaml$3124-3151

https://github.com/prometheus/statsd_exporter#statsd-timers-and-distributions

(something like:

mappings:
 - match: whatever this metric name will end up being
   observer_type: histogram
   histogram_options:
     buckets: [0, 1, 2, 3, 4, 5]

(you will want to test that in docker as well)

olasd: For all these histograms, we will want to change the configuration of the statsd exporter for…
)
for cls in fetcher_classes:
metadata_fetcher = cls( metadata_fetcher = cls(
origin=self.origin, origin=self.origin,
lister_name=self.lister_name, lister_name=self.lister_name,
lister_instance_name=self.lister_instance_name, lister_instance_name=self.lister_instance_name,
credentials=self.metadata_fetcher_credentials, credentials=self.metadata_fetcher_credentials,
) )
with self.statsd_timed("fetch_one_metadata"): with self.statsd_timed(
"fetch_one_metadata", tags={"fetcher": cls.FETCHER_NAME}
):
metadata.extend(metadata_fetcher.get_origin_metadata()) metadata.extend(metadata_fetcher.get_origin_metadata())
if self.parent_origins is None: if self.parent_origins is None:
self.parent_origins = metadata_fetcher.get_parent_origins() self.parent_origins = metadata_fetcher.get_parent_origins()
statsd.histogram(
f"{STATSD_PREFIX}.metadata.parent_origins",
len(self.parent_origins),
tags={"fetcher": cls.FETCHER_NAME, "visit_type": self.visit_type},
)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.objects",
len(metadata),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

Do we really want a histogram, or just a running total? (note that the histogram will also generate a running total)

olasd: Do we really want a histogram, or just a running total? (note that the histogram will also…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I don't know; I'm only interested in the average, and an histogram seemed like the only way to do that (ditto above)

vlorentz: I don't know; I'm only interested in the average, and an histogram seemed like the only way to…
olasdUnsubmitted
Done
Inline Actions

If you just want an average, then you just need to have a pair of metrics: one that is incremented for each operation (and will be shared across all metrics), and one that is incremented by the number of objects you want to count the average of. You can then divide these two to get a running average.

The prometheus statsd exporter just generates these series automatically for histograms, so that they can be used with the prometheus histogram functions: there's a <foo>_count series for the number of times a value was measured, and a <foo>_sum series for the sum of all <foo> values mesured, in addition to all the "bucket counts" series (which generate one series for the number of counts lower than a given bucket limit).

Either way, in terms of storage, as long as we pinpoint a small set of relevant buckets, it shouldn't matter too much. If you think that we're never going to need the fine-grained metrics (I probably agree), please use counters directly.

olasd: If you just want an average, then you just need to have a pair of metrics: one that is…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Done. I kept {name} inside the metric instead of tags; because it doesn't make sense to aggregate them (like in the other diff) IMO

vlorentz: Done. I kept `{name}` inside the metric instead of tags; because it doesn't make sense to…
olasdUnsubmitted
Not Done
Inline Actions

Yeah, that's perfect!

olasd: Yeah, that's perfect!
)
return metadata return metadata
@contextlib.contextmanager @contextlib.contextmanager
def statsd_timed(self, name, tags={}): def statsd_timed(self, name, tags={}):
with statsd.timed( with statsd.timed(
f"{STATSD_PREFIX}.duration_seconds.{name}", f"{STATSD_PREFIX}.duration_seconds.{name}",
tags={"visit_type": self.visit_type, **tags}, tags={"visit_type": self.visit_type, **tags},
▲ Show 20 Lines • Show All 85 Lines • Show Last 20 Lines
View Options

swh/web/tests/conftest.py

Show First 20 Lines • Show All 452 Lines • ▼ Show 20 Lines def build_extrinsic_origin_metadata(self) -> List[RawExtrinsicMetadata]:
self.log.debug("lister_not provided, skipping extrinsic origin metadata") self.log.debug("lister_not provided, skipping extrinsic origin metadata")
return [] return []
assert ( assert (
self.lister_instance_name is not None self.lister_instance_name is not None
), "lister_instance_name is None, but lister_name is not" ), "lister_instance_name is None, but lister_name is not"
metadata = [] metadata = []
for cls in get_fetchers_for_lister(self.lister_name):
fetcher_classes = get_fetchers_for_lister(self.lister_name)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.fetchers",
len(fetcher_classes),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

For all these histograms, we will want to change the configuration of the statsd exporter for this metric to generate a histogram with relevant buckets. Our default buckets are only relevant for timings and will be really wasteful for this (which will pretty much always return 0 or 1)

https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/data/common/common.yaml$3124-3151

https://github.com/prometheus/statsd_exporter#statsd-timers-and-distributions

(something like:

mappings:
 - match: whatever this metric name will end up being
   observer_type: histogram
   histogram_options:
     buckets: [0, 1, 2, 3, 4, 5]

(you will want to test that in docker as well)

olasd: For all these histograms, we will want to change the configuration of the statsd exporter for…
)
for cls in fetcher_classes:
metadata_fetcher = cls( metadata_fetcher = cls(
origin=self.origin, origin=self.origin,
lister_name=self.lister_name, lister_name=self.lister_name,
lister_instance_name=self.lister_instance_name, lister_instance_name=self.lister_instance_name,
credentials=self.metadata_fetcher_credentials, credentials=self.metadata_fetcher_credentials,
) )
with self.statsd_timed("fetch_one_metadata"): with self.statsd_timed(
"fetch_one_metadata", tags={"fetcher": cls.FETCHER_NAME}
):
metadata.extend(metadata_fetcher.get_origin_metadata()) metadata.extend(metadata_fetcher.get_origin_metadata())
if self.parent_origins is None: if self.parent_origins is None:
self.parent_origins = metadata_fetcher.get_parent_origins() self.parent_origins = metadata_fetcher.get_parent_origins()
statsd.histogram(
f"{STATSD_PREFIX}.metadata.parent_origins",
len(self.parent_origins),
tags={"fetcher": cls.FETCHER_NAME, "visit_type": self.visit_type},
)
statsd.histogram(
f"{STATSD_PREFIX}.metadata.objects",
len(metadata),
tags={"visit_type": self.visit_type},
olasdUnsubmitted
Done
Inline Actions

Do we really want a histogram, or just a running total? (note that the histogram will also generate a running total)

olasd: Do we really want a histogram, or just a running total? (note that the histogram will also…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I don't know; I'm only interested in the average, and an histogram seemed like the only way to do that (ditto above)

vlorentz: I don't know; I'm only interested in the average, and an histogram seemed like the only way to…
olasdUnsubmitted
Done
Inline Actions

If you just want an average, then you just need to have a pair of metrics: one that is incremented for each operation (and will be shared across all metrics), and one that is incremented by the number of objects you want to count the average of. You can then divide these two to get a running average.

The prometheus statsd exporter just generates these series automatically for histograms, so that they can be used with the prometheus histogram functions: there's a <foo>_count series for the number of times a value was measured, and a <foo>_sum series for the sum of all <foo> values mesured, in addition to all the "bucket counts" series (which generate one series for the number of counts lower than a given bucket limit).

Either way, in terms of storage, as long as we pinpoint a small set of relevant buckets, it shouldn't matter too much. If you think that we're never going to need the fine-grained metrics (I probably agree), please use counters directly.

olasd: If you just want an average, then you just need to have a pair of metrics: one that is…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Done. I kept {name} inside the metric instead of tags; because it doesn't make sense to aggregate them (like in the other diff) IMO

vlorentz: Done. I kept `{name}` inside the metric instead of tags; because it doesn't make sense to…
olasdUnsubmitted
Not Done
Inline Actions

Yeah, that's perfect!

olasd: Yeah, that's perfect!
)
return metadata return metadata
@contextlib.contextmanager @contextlib.contextmanager
def statsd_timed(self, name, tags={}): def statsd_timed(self, name, tags={}):
with statsd.timed( with statsd.timed(
f"{STATSD_PREFIX}.duration_seconds.{name}", f"{STATSD_PREFIX}.duration_seconds.{name}",
tags={"visit_type": self.visit_type, **tags}, tags={"visit_type": self.visit_type, **tags},
▲ Show 20 Lines • Show All 85 Lines • Show Last 20 Lines
View Options

swh/web/tests/data.py

Loading...
View Options

swh/web/tests/strategies.py

Loading...
About · Developer information · Legal