add metrics for number of tests

[yachang]

public & private

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 480

• 7 of 11 (63.64%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.06%) to 44.321%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
embargo.go	7	9	77.78%
metrics/metrics.go	0	2	0.0%
<!--	Total:	7	11	63.64%	-->

Totals
Change from base Build 472:	0.06%
Covered Lines:	320
Relevant Lines:	722

---

💛 - Coveralls

[stephen-soltesz]

Hey, Ya. In this PR, I'm asking you to change some of the patterns originally established in https://github.com/m-lab/etl-embargo/pull/10 which I also reviewed. While I regret my inconsistency there, I do believe that my suggestions now are more in line with the published best practices for prometheus metrics and with our evolving usage of them. Let me know if this is a problem.

---

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

metrics/metrics.go, line 45 at r1 (raw file):

  // Example usage:
  //   metrics.Metrics_embargoPublicTestTotal.WithLabelValues("sidestream", dayOfWeek).Inc()
  Metrics_embargoPublicTestTotal = prometheus.NewCounterVec(

Please combine Metrics_embargoPublicTestTotal and Metrics_embargoPrivateTestTotal into a single metric, with a name like Metrics_embargoTestTotal.

Then add a new label for "visbility" with two possible values, either "public" or "private".

This approach is closer to best-practice for prometheus metrics.

---

metrics/metrics.go, line 47 at r1 (raw file):

  Metrics_embargoPublicTestTotal = prometheus.NewCounterVec(
      prometheus.CounterOpts{
          Name: "embargo_Public_Test_Total",

Please change new metrics to use all lower-case. e.g. `"embargo_test_total". I should have caught this in earlier PRs.

---

metrics/metrics.go, line 51 at r1 (raw file):

      },
      // "sidestream", "Monday"
      []string{"experiment", "day_of_week"})

Do we need the day_of_week label? The dashboard you've created does not use it. I'm imagining we'll use this metric like we use the "Daily Volume" metric.

---

metrics/metrics.go, line 51 at r1 (raw file):

      },
      // "sidestream", "Monday"
      []string{"experiment", "day_of_week"})

Scraper uses label name "rsync_host_module" with a value "sidestream". ETL parsers use label name "table" with value "sidestream". Many platform probes (e.g. to NDT, neubot, etc) use label name "experiment" with values corresponding to the DNS prefix, e.g. "experiment=ndt.iupui", "experiment=utility.mlab", experiment="neubot.mlab", etc.

I'm worried about reusing the experiment label with different kinds of values. I think of label names as "data type" because those are the dimension along which we join different metrics in promql queries.

Instead, can we say something like the ETL-Embargo is operating on "datasets"? Could we call this label "dataset" with a value of "sidestream"?

---

Comments from Reviewable

[yachang]

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

---

metrics/metrics.go, line 45 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please combine `Metrics_embargoPublicTestTotal` and `Metrics_embargoPrivateTestTotal` into a single metric, with a name like `Metrics_embargoTestTotal`. Then add a new label for "visbility" with two possible values, either "public" or "private". This approach is closer to best-practice for prometheus metrics.

Done.

---

metrics/metrics.go, line 47 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please change new metrics to use all lower-case. e.g. `"embargo_test_total". I should have caught this in earlier PRs.

Done.

---

metrics/metrics.go, line 51 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Do we need the `day_of_week` label? The dashboard you've created does not use it. I'm imagining we'll use this metric like we use the "Daily Volume" metric.

Done.

---

metrics/metrics.go, line 51 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Scraper uses label name "rsync_host_module" with a value "sidestream". ETL parsers use label name "table" with value "sidestream". Many platform probes (e.g. to NDT, neubot, etc) use label name "experiment" with values corresponding to the DNS prefix, e.g. "experiment=ndt.iupui", "experiment=utility.mlab", experiment="neubot.mlab", etc. I'm worried about reusing the experiment label with different kinds of values. I think of label names as "data type" because those are the dimension along which we join different metrics in promql queries. Instead, can we say something like the ETL-Embargo is operating on "datasets"? Could we call this label "dataset" with a value of "sidestream"?

Done.

---

Comments from Reviewable

[stephen-soltesz]

---

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable