add tar file output count

yachang commented 6 years ago

This change is

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 489

7 of 12 (58.33%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.5%) to 44.842%

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

Totals
Change from base Build 481:	0.5%
Covered Lines:	326
Relevant Lines:	727

💛 - Coveralls

stephen-soltesz commented 6 years ago

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

embargo.go, line 120 at r1 (raw file):

  } else {
                metrics.Metrics_embargoTarOutputTotal.WithLabelValues("sidestream", "private").Inc()
        }

go fmt please.

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

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

Please name this metric with "Input" so it's clear that it complements the output metric Metrics_embargoTarOutputTotal.

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

  //   embargo_tar_output_total
  // Example usage:
  //   metrics.Metrics_embargoTarOutputTotal.WithLabelValues("sidestream", "status").Inc()

Please use an example status value like "public" in this comment. Please update the other comment examples for Metrics_embargoTarTotal and Metrics_embargoTestTotal.

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

      prometheus.CounterOpts{
          Name: "embargo_tar_output_total",
          Help: "Number of tar files that were processed by embargo app engine.",

Please update the Help text to be distinct from Metrics_embargoTarTotal.

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

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

So, we discovered looking at the dashboard this morning that this metric is actually counting files. Does it makes sense to name this metric after "files" rather than "tests"?

If future experiments want to support embargo, we should require that the criteria for filtering be simple, file-based criteria that do not require parsing the files. So, I think something like "embargo_file_total" could apply to future data also.

Comments from Reviewable

yachang commented 6 years ago

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

embargo.go, line 120 at r1 (raw file):