Change Metrics Collection Interval to time.Duration #282

braydonk · 2021-12-02T14:57:56Z

#276 updated the goccy/go-yaml library to a version that parses time.Duration directly, and the new time field used that. Update MetricsReceiverShared.CollectionInterval to be a time.Duration for consistency (also resolves this TODO).

Adds new field wildcard_refresh_interval to the logging files receiver in the ops-agent config.

Use a different strategy for the validator that is more scalable than the original `whole_time_denomination` implementation; take a time.Duration and see if it is a multiple of some other duration.

igorpeshansky · 2021-12-02T16:48:14Z

confgenerator/built-in-config-linux.yaml

@@ -13,7 +13,7 @@ metrics:
  receivers:
    hostmetrics:
      type: hostmetrics
-      collection_interval: 60s
+      collection_interval: 1m0s


Bleh. Is there no way we can force this to be serialized as seconds? These are user-visible sample configs, so it is not a great idea to change them…

Looks like the goccy/go-yaml library does not support this, but maybe a custom marshaler?

ISTR last I looked into this it was not possible to implement a custom marshaler (hence why I had to fix it upstream in the first place). I think we just have to live with this; this is the canonical serialization of a time.Duration(60000000000) in Go.

I recall a GitHub issue in the golang/go repo where custom time.Duration string formatting was discussed, and it was denied because of backwards compatibility. I'm going to keep looking for the issue, I wish I'd saved it. But the outcome of that was that we'll have to have some custom function to format this as 60s if we want it that way.

The last intrusive way I can think to do that would be to keep the CollectionIntervalString function, check if the value is 60 seconds, and deliberately output 60s in that scenario, otherwise just return the string formatted duration.

I think we just need to live with it; this is the canonical serialization of 60s.

igorpeshansky · 2021-12-02T16:49:37Z

confgenerator/config.go

-	if m.CollectionInterval != "" {
-		return m.CollectionInterval
-	}
-	return "60s"


Did we just remove the 60s default? Does it matter?

+1 (to Igor's comment, that is, not the removal of the code)

I put 60 seconds as a value any time it looked like we relied on the default result of CollectionIntervalString, did I miss any?
I will change to using a constant instead of putting 60 seconds everywhere.

Yeah, a constant would be preferable, thanks!

igorpeshansky · 2021-12-02T16:51:48Z

...erator/testdata/valid/linux/all-backward_compatible_with_explicit_exporters/golden_otel.conf

@@ -344,7 +344,7 @@ processors:
    - gce
 receivers:
  hostmetrics/default__pipeline_hostmetrics:
-    collection_interval: 60s
+    collection_interval: 1m0s


[Optional] This one is not ideal either, but less annoying than the user-visible sample config change.

igorpeshansky · 2021-12-02T16:55:35Z

.../invalid/linux/metrics-receiver_invalid_parameter_malformed_collection_interval/golden_error

[Optional] Sigh. This is a net negative for usability, but there's precedent for messages like this, so we can probably kick that can down the road and address all of those in one fell swoop at some point…

Yeah, fixing this generically for all fields would be a great separate PR.

quentinmit · 2021-12-03T04:44:04Z

apps/builtinconf.go

@@ -39,7 +43,7 @@ var (
 				Receivers: map[string]cg.MetricsReceiver{
 					"hostmetrics": &MetricsReceiverHostmetrics{
 						ConfigComponent:       cg.ConfigComponent{Type: "hostmetrics"},
-						MetricsReceiverShared: cg.MetricsReceiverShared{CollectionInterval: "60s"},
+						MetricsReceiverShared: cg.MetricsReceiverShared{CollectionInterval: time.Second * 60},


Nit: It reads better if the number comes first.

Suggested change

MetricsReceiverShared: cg.MetricsReceiverShared{CollectionInterval: time.Second * 60},

MetricsReceiverShared: cg.MetricsReceiverShared{CollectionInterval: 60 * time.Second},

quentinmit · 2021-12-03T04:44:56Z

confgenerator/config.go

-	if m.CollectionInterval != "" {
-		return m.CollectionInterval
-	}
-	return "60s"


+1 (to Igor's comment, that is, not the removal of the code)

braydonk added 6 commits November 22, 2021 15:55

confgenerator: add wildcard_refresh_interval

b95a9dd

Adds new field wildcard_refresh_interval to the logging files receiver in the ops-agent config.

confgenerator: added 3rd party receiver tests

cf6f4a0

confgenerator: removed unused casting case

1c8bad4

confgenerator: change validator to multipleof_time

5eb2441

Use a different strategy for the validator that is more scalable than the original `whole_time_denomination` implementation; take a time.Duration and see if it is a multiple of some other duration.

confgenerator: fixed copyright comment

0d2aa4e

rebase on braydonk-refresh-interval

1f10ccd

braydonk force-pushed the braydonk-collection-interval-duration branch from ae2815f to 1f10ccd Compare December 2, 2021 15:00

confgenerator: remove unused duration case

f4750bd

igorpeshansky requested changes Dec 2, 2021

View reviewed changes

igorpeshansky requested a review from quentinmit December 2, 2021 16:59

quentinmit reviewed Dec 3, 2021

View reviewed changes

braydonk changed the base branch from braydonk-refresh-interval to master December 21, 2021 13:33

braydonk force-pushed the braydonk-collection-interval-duration branch from 6857f5c to f4750bd Compare December 21, 2021 13:59

braydonk added 2 commits December 21, 2021 14:02

Merge master to fix merge conflicts

3d76ebe

apps: use defaultCollectionInterval variable

312a00f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Metrics Collection Interval to time.Duration #282

Change Metrics Collection Interval to time.Duration #282

braydonk commented Dec 2, 2021

igorpeshansky Dec 2, 2021

quentinmit Dec 3, 2021

braydonk Dec 7, 2021

quentinmit Dec 21, 2021

igorpeshansky Dec 2, 2021

quentinmit Dec 3, 2021 •

edited

Loading

braydonk Dec 7, 2021

quentinmit Dec 21, 2021

igorpeshansky Dec 2, 2021

igorpeshansky Dec 2, 2021

quentinmit Dec 3, 2021

quentinmit Dec 3, 2021

quentinmit Dec 3, 2021 •

edited

Loading

	MetricsReceiverShared: cg.MetricsReceiverShared{CollectionInterval: time.Second * 60},
	MetricsReceiverShared: cg.MetricsReceiverShared{CollectionInterval: 60 * time.Second},

Change Metrics Collection Interval to time.Duration #282

Are you sure you want to change the base?

Change Metrics Collection Interval to time.Duration #282

Conversation

braydonk commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quentinmit Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quentinmit Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

quentinmit Dec 3, 2021 •

edited

Loading

quentinmit Dec 3, 2021 •

edited

Loading