Add collection-interval to metrics-generator user-configurable overrides

[rlankfo]

What this PR does:

Extends the user-configurable overrides added in #2543 with collection_interval override.

New JSON payload:

{
    "forwarders": [],
    "metrics_generator": {
        "processors": [
            "service-graphs",
            "span-metrics"
        ],
        "disable_collection": false,
        "collection_interval": "60s",
        "processor": {
            "service_graphs": {
                "dimensions": [
                    "service.namespace"
                ],
                "enable_client_server_prefix": true,
                "peer_attributes": [
                    "peer.service",
                    "db.name",
                    "db.system",
                    "db.url"
                ]
            },
            "span_metrics": {
                "dimensions": [
                    "service.namespace"
                ],
                "enable_target_info": true
            }
        }
    }
}

Which issue(s) this PR fixes:
Fixes #

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[CLAassistant]

All committers have signed the CLA.

[rlankfo]

The json decoder expects time.Duration fields to be passed in nanoseconds. This means 60s is represented as 60000000000. This seemed like an acceptable trade-off and cleaner than making this a string and calling time.ParseDuration.

[kvrhdn]

This should be fine, we use time.Duration in a lot of configuration structs and it will automatically parse durations formatted like 15s or 1m.

[rlankfo]

I believe that will work when unmarshaling yaml because it's handled here:
https://github.com/go-yaml/yaml/blob/v3.0.1/decode.go#L647-L653

The json decoder won't accept a non numeric value, such as "60s". So I think here we'll either have to use nanoseconds as I pointed out, or this would need to be a string and we'd have to figure out the best place to run time.ParseDuration.

[rlankfo]

Another option could be to change the JSON field to "collection_interval_seconds" and return *l.CollectionInterval * time.Second in GetCollectionInterval

[kvrhdn]

Oh I see, it's an oversight of the json decoder to not use time.ParseDuration. I'd prefer to implement our own unmarshaler in that case so we can use 15s and 1m etc. That is easier to use and consistent with configuration in the configmap.

There are articles explaining how to implement the unmarshaler interface: https://biscuit.ninja/posts/go-unmarshalling-json-into-time-duration/

[rlankfo]

thanks @kvrhdn. I implemented the custom marshal/unmarshal and added some tests. Let me know what you think!

[kvrhdn]

The changes look good, thanks!

Can you add some validation around collection interval? While users are free to set their preferred interval, we have to make sure they don't break anything or stress Tempo too much.
We have a validator for the overrides in this file: https://github.com/grafana/tempo/blob/main/cmd/tempo/app/overrides_validation.go

Right now people could post negative values which might lead to a panic. I think it's reasonable to say collection interval must be a value between 15s and 5m?

[kvrhdn]

This should be fine, we use time.Duration in a lot of configuration structs and it will automatically parse durations formatted like 15s or 1m.

[rlankfo]

Right now people could post negative values which might lead to a panic. I think it's reasonable to say collection interval must be a value between 15s and 5m?

Thanks for the review @kvrhdn. I've added validation for this, please take a look.

[kvrhdn]

LGTM, works as expected 👍🏻 Just a slight comment about file structure.

[kvrhdn]

Mostly a nitpick: I think it would be cleaner to move the Duration struct into a separate file:

• this file (unfortunately) already has a lot of boilerplate making it difficult to read
• keeping it in a separate file will make it easier to reuse later, we can just move the file to shared package when we need it

What do you think of moving this struct and the corresponding test functions into duration.go?

[rlankfo]

Makes sense to me. I moved the code. Thanks!

[kvrhdn]

LGTM, thanks!