Define and register plugin factories for datalayer #1911
Conversation
- Factories for metrics DataSource and Extracor. - Configuration parameters (with defaults set from command line flags). - Consistently use typedName instead of `tn`. Signed-off-by: Etai Lev Ran <[email protected]>
k8s-ci-robot
added
the
kind/feature
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
size/L
Signed-off-by: Etai Lev Ran <[email protected]>
| extractorParams struct { | ||
| // QueueRequestsSpec defines the metric specification string for retrieving queued request count. | ||
| QueueRequestsSpec string // `json:"queuedRequestsSpec"` | ||
| // RunningRequestsSpec defines the metric specification string for retrieving running requests count. | ||
| RunningRequestsSpec string // `json:"runningRequestsSpec"` | ||
| // KVUsage defines the metric specification string for retrieving KV cache usage. | ||
| KVUsageSpec string // `json:"kvUsageSpec"` | ||
| // LoRASpec defines the metric specification string for retrieving LoRA availability. | ||
| LoRASpec string // `json:"loraSpec"` | ||
| // CacheInfoSpec defines the metrics specification string for retrieving KV cache configuration. | ||
| CacheInfoSpec string // `json:"cacheInfoSpec"` | ||
| } |
There was a problem hiding this comment.
can we make this as general key value pairs?
Ideally I think we don't want to change code and rebuild every time we update the metrics we're collecting.
(e.g., different orgs may have different plugins that require different metrics).
There was a problem hiding this comment.
These are configuration items for the specific Extractor we have today and is not meant for a generic metrics collection plugin. There are no plans for a generic metrics extractor that accepts a set of arbitrary metric names and extracts them.
Each metric spec is later interpreted differently (e.g., to get the latest, LORA and queue sizes are handled differently).
Also, making it generic KV-pairs implies we need to define and validate the keys, where the current params makes it explicit and matches the specific metrics collected.
If an aorganization needs new metrics they should code and register a new extractor attached to the existing metrics data source. That extractor would have its own configuration section, if needed.
There was a problem hiding this comment.
I think for the long term it might be good to investigate if we can make this a generic key/value pairs (maybe to include also the type of the metric, e.g., int, float?)
anyway, this is not a blocker for now for sure.
Signed-off-by: Etai Lev Ran <[email protected]>
|
@shmuelk changed names to add specificity (e.g., MetricsDataSource, ModelServerExtractor). |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elevran, nirrozenbaum, shmuelk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds the needed foundation for integrating datalayer with the configuration file. It builds on initial plugin support in #1901.
datalayer.metrics.typedNameandbackendmetrics.AllPodsPredicate)paramssection.Note that the parameters are currently provided via CLI flags. The code uses the flags to set default configuration values before attempting to parse the configuration section. The change is not as clean as I'd like as it duplicates the flag names.
A better solution could have to concentrate all flag names and default values in a new
epp/clipackage (currently flags names are incmd/epp/runnerand their defaults are inpkg/epp/server). I can open an issue/PR if that sounds reasonable to do.A follow up PR will add deprecation notice to the flags being replaced by configuration parameters. This will be done once config loading is ready. For additional context, see comments in datalayer/metrics/factories.go L100-110 of this PR.
Which issue(s) this PR fixes:
Refs #1408
Refs #1883
Refs #1910
Does this PR introduce a user-facing change?:
Not yet. A follow up PR would tie this into the configuration loading and deprecate CLI flags, both of which are user facing.