-
Notifications
You must be signed in to change notification settings - Fork 59
Add a generic labeled gauge for K8s plugin resources #137
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Co-authored-by: Haytham AbuelFutuh <[email protected]>
Co-authored-by: Haytham AbuelFutuh <[email protected]>
…ller into plugin-manager-collector
Co-authored-by: Haytham AbuelFutuh <[email protected]>
This reverts commit d21aacf.
Scope promutils.Scope | ||
|
||
// Meta timer - this times each collection cycle to measure how long it takes to collect the levels GaugeVec below | ||
CollectorTimer *labeled.StopWatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a prometheus collector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not have you considered using - https://godoc.org/github.com/prometheus/client_golang/prometheus#Collector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stopwatches are defined in flytestdlib, but they're basically Summary objects https://github.com/lyft/flytestdlib/blob/c963e9ff8fc9bb46f169c43b923af07d6c5bf71e/promutils/scope.go#L72
} | ||
|
||
func (r *ResourceLevelMonitor) RunCollector(ctx context.Context) { | ||
ticker := time.NewTicker(resourceLevelMonitorCycleDuration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defer ticker.Stop()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean inside the goroutine right? If I add it here, the ticker will just stop and it'll never run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added it inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Ketan Umare <[email protected]>
# TL;DR Adds a generic labeled gauge for K8s plugin resources ## Type - [ ] Bug Fix - [x] Feature - [ ] Plugin ## Are all requirements met? - [x] Code completed - [x] Smoke tested - [x] Unit tests added - [x] Code documentation added - [x] Any pending items have an associated Issue ## Complete description * We are adding a generic utility to emit gauge metrics in the `plugin_manager` using the labeled gauges now found in flytestdlib. * Henceforth, a goroutine will be spun up that will create a `ResourceLevelMonitor` for each type that a plugin registers. * Changing the existing gauge collecting utilty for FlyteWorkflow CRD objects to use the same labeled gauge. * We may wish to combine these in the future but as their aggregations are different, we are keeping them separate for now. ## Tracking Issue flyteorg/flyte#311 ## Follow-up issue NA
TL;DR
Adds a generic labeled gauge for K8s plugin resources
Type
Are all requirements met?
Complete description
plugin_manager
using the labeled gauges now found in flytestdlib.ResourceLevelMonitor
for each type that a plugin registers.Tracking Issue
flyteorg/flyte#311
Follow-up issue
NA