Support Spinnaker's various TimeUnit's

[karlskewes]

Spinnaker code doesn't just use TimeUnit.SECONDS throughout the code, various incantations such as TimeUnit.MILLISECONDS and TimeUnit.NANOSECONDS are also used.

Example: https://github.com/spinnaker/kork/blob/master/kork-sql/src/main/kotlin/com/netflix/spinnaker/kork/sql/telemetry/HikariSpectatorMetricsTracker.kt#L52

registry.timer(connectionAcquiredId).record(elapsedAcquiredNanos, TimeUnit.NANOSECONDS)

In the plugin, we only use seconds and thus our metric values can be off.

armory-observability-plugin/common/src/main/java/io/micrometer/prometheus/MutatedPrometheusMeterRegistry.java

Lines 432 to 434 in 9bcb277

	protected TimeUnit getBaseTimeUnit() {
	return TimeUnit.SECONDS;
	}

I don't know if we can reliably determine the unit of time and include in our metric name per Prometheus Best Practices.

Potential options:

1. Drop time unit from name (eg: _seconds) and require users of the plugin to discover the unit through Spinnaker source code? I am happy to work through the spinnaker-mixin dashboards and alerts.
2. Find some way to parse the time unit?
3. Do nothing?
4. Other?

[jasonmcintosh]

SO never did respond to this directly...

• I'd be completely good with removing _seconds - some of the existing conventions are rather goofy unfortunately and not consistent.
• MIGHT need to tweak various registry configs & other settings
• There are some Counters that are treated more like gauges in how they operate because of threading or other configs.
• ANOTHER issue is ... this is a wrapper around Spectator - there's a case where maybe we could calculate some of these by seeing if it's a Spectator metric & if there's a Time unit associated and mutating the name (or value) to match standards better...

Guess long story short - yeah this is problematic, but would require some additional thought/solutioning...

[karlskewes]

Thanks Jason. Makes a lot of sense.
Per Slack conversations it seems we're good to migrate the Milliseconds metrics to Nanoseconds which would make upstream Spectator metrics consistent but might not absolve the need to parse the name I guess.
Need to do a deep dive at some point. :)