lib/periodic: add function to inject metrics in task

[Hygster]

Add StartWithMetrics function to the periodic task library to have full control over the metrics that are exposed by the library.

---

This change is 

[lukedirtwalker]

Reviewed 6 of 9 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @Hygster)

a discussion (no related file):

Fixes #5785

Please remove this reference, it doesn't have much meaning here since this is an internal ticket.

---

a discussion (no related file):
title:
lib/periodic: add function to inject metrics in task

description:

Add StartWithMetrics function to the periodic task library to have full control over the metrics that are exposed by the library.

---

---

gateway/gateway.go line 318 at r3 (raw file):

	// periodicly clean up the revocation store.
	revCleaner := periodic.StartWithMetric(

actually let's keep the old implementation anyway so that we don't introduce any unexpected changes.

---

private/file/periodicview.go line 86 at r3 (raw file):

		v.readTask.read()

		// Set up metrics for goroutine

Why are the changes in this file? can't we just use the periodic.Start still for this?

---

private/periodic/periodic.go line 24 at r3 (raw file):

	"github.com/scionproto/scion/pkg/log"
	emetrics "github.com/scionproto/scion/pkg/metrics"

Instead of naming this import let's change the name of the old import to legacymetrics

---

private/periodic/periodic.go line 47 at r3 (raw file):

)

// Metrics contins the relavant metrics for the task in the same Runner.

Suggestion:

contains

---

private/periodic/periodic.go line 47 at r3 (raw file):

)

// Metrics contins the relavant metrics for the task in the same Runner.

Suggestion:

for a periodic task.

---

private/periodic/periodic.go line 59 at r3 (raw file):

}

func (m Metrics) SetStartTimestamp(t time.Time) {

all those methods don't have to be public since they are just used here.

(applies to all 4 methods)

---

private/periodic/periodic.go line 60 at r3 (raw file):

func (m Metrics) SetStartTimestamp(t time.Time) {
	if m.StartTime != nil {

you can use the metrics helpers for those methods then you don't need the if:

	emetrics.GaugeSet(m.StartTime, float64(t.UnixNano() / 1e9))

(applies to all 4 methods)

---

private/periodic/periodic.go line 183 at r3 (raw file):

}

// Start creates and starts a new Runner to run the given task peridiocally.

Since Start and StartWithMetrics are kind of constructors for the Runner struct let's move them directly after the declaration of the struct.

---

private/periodic/periodic.go line 187 at r3 (raw file):

// larger than the periodicity of the task. That means if a tasks takes a long
// time it will be immediately retriggered.
func Start(task Task, period, timeout time.Duration) *Runner {

let's mark this as deprecated:

// Deprecated: Start exists for compatibility reasons, use StartWithMetrics instead

see also (https://github.com/golang/go/wiki/Deprecated)

---

private/periodic/periodic.go line 200 at r3 (raw file):

// StartWithMetric is identical to Start but allows the caller to
// specify the metric or no metric at all to be used.
func StartWithMetric(task Task, metric Metrics, period, timeout time.Duration) *Runner {

Suggestion:

 StartWithMetrics

---

private/periodic/periodic.go line 216 at r3 (raw file):

	}
	logger.Info("Starting periodic task", "task", task.Name())
	if r.metric.Period != nil {

why not use the r.metric.setPeriod() method? or if we don't want to use it we can just use the metrics.GaugeSet function

---

private/periodic/periodic.go line 219 at r3 (raw file):

		r.metric.Period.Set(period.Seconds())
	}
	if r.metric.StartTime != nil {

ditto

---

private/periodic/periodic_test.go line 55 at r3 (raw file):

	p := time.Duration(want) * 20 * time.Millisecond
	r := StartWithMetric(fn, met, p, time.Hour)
	// defer func() {

let's drop this.

---

private/periodic/periodic_test.go line 173 at r3 (raw file):

		},
		// With additional metrics
		Period:    metrics.NewTestGauge(),

can we also check the values of those at the end.

You can use https://pkg.go.dev/github.com/stretchr/testify/assert?utm_source=godoc#InDelta for the Time metrics.

---

private/periodic/internal/metrics/metrics.go line 28 at r3 (raw file):

// ExportMetric is the interface to export periodic metrics.
type ExportMetric interface {

let's drop this, it's no longer used.

---

private/periodic/internal/metrics/metrics.go line 40 at r3 (raw file):

}

type exporter struct {

I think we can restructure this quite a bit I would just create a public struct that contains all the fields you have here as public fields, then you don't need the Get... methods.

Also I think we can just call this strcut Metrics then and only have a single New function.

[Hygster]

Reviewable status: 3 of 9 files reviewed, 18 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Fixes #5785

Please remove this reference, it doesn't have much meaning here since this is an internal ticket.

Is that always the case when working on the scionproto repo?
Done.

---

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

title:
lib/periodic: add function to inject metrics in task

description:

Add StartWithMetrics function to the periodic task library to have full control over the metrics that are exposed by the library.

Done.

---

---

gateway/gateway.go line 318 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

actually let's keep the old implementation anyway so that we don't introduce any unexpected changes.

Done.

Code quote:

	// periodicly clean up the revocation store.
	revCleaner := periodic.Start(periodic.Func{

		Task: func(ctx context.Context) {
			revStore.Cleanup(ctx)
		},
		TaskName: "revocation_store_cleaner",
	}, 30*time.Second, 30*time.Second)

---

private/periodic/periodic.go line 24 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Instead of naming this import let's change the name of the old import to legacymetrics

Done.

---

private/periodic/periodic.go line 47 at r3 (raw file):

)

// Metrics contins the relavant metrics for the task in the same Runner.

just got a spell checker extension, so hopefully, that'll be the last time something like this slips through 😅

---

private/periodic/periodic.go line 47 at r3 (raw file):

)

// Metrics contins the relavant metrics for the task in the same Runner.

Done.

---

private/periodic/periodic.go line 59 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

all those methods don't have to be public since they are just used here.

(applies to all 4 methods)

Done.

---

private/periodic/periodic.go line 60 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

you can use the metrics helpers for those methods then you don't need the if:

	emetrics.GaugeSet(m.StartTime, float64(t.UnixNano() / 1e9))

(applies to all 4 methods)

Done.

---

private/periodic/periodic.go line 183 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Since Start and StartWithMetrics are kind of constructors for the Runner struct let's move them directly after the declaration of the struct.

Done.

---

private/periodic/periodic.go line 187 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

let's mark this as deprecated:

// Deprecated: Start exists for compatibility reasons, use StartWithMetrics instead

see also (https://github.com/golang/go/wiki/Deprecated)

Done.

---

private/periodic/periodic.go line 200 at r3 (raw file):

// StartWithMetric is identical to Start but allows the caller to
// specify the metric or no metric at all to be used.
func StartWithMetric(task Task, metric Metrics, period, timeout time.Duration) *Runner {

Done.

---

private/periodic/periodic.go line 216 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

why not use the r.metric.setPeriod() method? or if we don't want to use it we can just use the metrics.GaugeSet function

Done.

---

private/periodic/periodic.go line 219 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ditto

Done.

---

private/periodic/periodic_test.go line 55 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

let's drop this.

Done.

---

private/periodic/periodic_test.go line 173 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

can we also check the values of those at the end.

You can use https://pkg.go.dev/github.com/stretchr/testify/assert?utm_source=godoc#InDelta for the Time metrics.

Done.

---

private/periodic/internal/metrics/metrics.go line 28 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

let's drop this, it's no longer used.

Done.

---

private/periodic/internal/metrics/metrics.go line 40 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think we can restructure this quite a bit I would just create a public struct that contains all the fields you have here as public fields, then you don't need the Get... methods.

Also I think we can just call this strcut Metrics then and only have a single New function.

Done.

---

private/file/periodicview.go line 86 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Why are the changes in this file? can't we just use the periodic.Start still for this?

This, and the change in gateway.go, were the example implementations of StartWithMetrics you asked me to do. Maybe I misunderstood: I thought you wanted me to make them a part of the PR. I've reverted this now as well

[lukedirtwalker]

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Hygster)

a discussion (no related file):

Previously, Hygster (August Rønberg) wrote…

Is that always the case when working on the scionproto repo?
Done.

Yes internal references never work. If there is a public issue then you can also a Fixes #<public-issue-number> comment.

---

---

private/periodic/periodic.go line 47 at r3 (raw file):

Previously, Hygster (August Rønberg) wrote…

just got a spell checker extension, so hopefully, that'll be the last time something like this slips through 😅

💯

---

private/periodic/periodic.go line 48 at r4 (raw file):

// Metrics contains the relevant metrics for a periodic task.
type Metrics struct {

The fields of the struct still have to be public, only the set... method below can be non-public.
Otherwise users of the package can no longer specify the metrics.

---

private/periodic/periodic.go line 111 at r4 (raw file):

// time it will be immediately retriggered.
//
// Deprecated: Start exists for compatibility reasons, use StartWithMetrics instead

Suggestion:

instead.

---

private/periodic/periodic.go line 142 at r4 (raw file):

	logger.Info("Starting periodic task", "task", task.Name())
	r.metric.setPeriod(period)
	// metrics.GaugeSet(r.metric.period, period.Seconds())

remove

---

private/periodic/periodic_test.go line 15 at r4 (raw file):

// limitations under the License.

package periodic

could you try to make this periodic_test ?

---

private/periodic/internal/metrics/metrics.go line 28 at r4 (raw file):

// Metrics is the standard metrics used in periodic.Runner

// Deprecated: Metrics is used only my the deprecated function periodic.Start

Suggestion:

in the 

---

private/file/periodicview.go line 86 at r3 (raw file):

Previously, Hygster (August Rønberg) wrote…

This, and the change in gateway.go, were the example implementations of StartWithMetrics you asked me to do. Maybe I misunderstood: I thought you wanted me to make them a part of the PR. I've reverted this now as well

Yes that's correct, but I actually realized that this would be a slightly breaking change because the metrics would suddenly change, therefore I thought it's cleaner to not do it. We verify in the test that the things work as expected. Sorry for the back and forth.

[Hygster]

Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker)

---

private/periodic/periodic.go line 48 at r4 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

The fields of the struct still have to be public, only the set... method below can be non-public.
Otherwise users of the package can no longer specify the metrics.

Done.

---

private/periodic/periodic.go line 111 at r4 (raw file):

// time it will be immediately retriggered.
//
// Deprecated: Start exists for compatibility reasons, use StartWithMetrics instead

Done.

---

private/periodic/periodic.go line 142 at r4 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

remove

Done.

---

private/periodic/periodic_test.go line 15 at r4 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

could you try to make this periodic_test ?

Done.

---

private/periodic/internal/metrics/metrics.go line 28 at r4 (raw file):

// Metrics is the standard metrics used in periodic.Runner

// Deprecated: Metrics is used only my the deprecated function periodic.Start

Sadly the spell checker doesn't catch those mistakes 😅

---

private/file/periodicview.go line 86 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Yes that's correct, but I actually realized that this would be a slightly breaking change because the metrics would suddenly change, therefore I thought it's cleaner to not do it. We verify in the test that the things work as expected. Sorry for the back and forth.

I see, No worries

[lukedirtwalker]

Reviewed 4 of 4 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Hygster)

---

private/periodic/periodic_test.go line 42 at r7 (raw file):

func TestPeriodicExecution(t *testing.T) {
	events := metrics.NewTestCounter()
	met := periodic.Metrics{

let's just name this m everywhere.

Suggestion:

m := periodic.Metrics

---

private/periodic/periodic_test.go line 81 at r7 (raw file):

	assert.NoError(t, err, "r.Stop() action timed out")
	// Check that  metrics work as expected
	assert.Equal(t, float64(1), metrics.CounterValue(r.GetMetric().Events(periodic.EventStop)))

here you can just refer to the metrics struct (met or after renaming m). Then you don't need the export_test.go

Suggestion:

m

[lukedirtwalker]

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Hygster)

[lukedirtwalker]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Hygster)

a discussion (no related file):
You can rm -r private/periodic/internal/metrics/mock_metrics this should no longer be needed.

---

a discussion (no related file):
There are some golangci-lint reports here https://buildkite.com/scionproto/scion/builds/2890#018160af-ec54-489f-ad7c-3bf27925dcc4/153-326

---

[Hygster]

Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

You can rm -r private/periodic/internal/metrics/mock_metrics this should no longer be needed.

Done.

---

a discussion (no related file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

There are some golangci-lint reports here https://buildkite.com/scionproto/scion/builds/2890#018160af-ec54-489f-ad7c-3bf27925dcc4/153-326

Done.

---

[lukedirtwalker]

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Hygster)