[v8] Use embedded process instances for "cf apps" summary#3725
[v8] Use embedded process instances for "cf apps" summary#3725jochenehret wants to merge 3 commits intocloudfoundry:v8from
Conversation
* use "/v3/processes?space_guids=:guid&embed=process_instances" to get processes and process instances in one request * see cloudfoundry/cloud_controller_ng#4796
c4b3d1c to
dea08a9
Compare
|
Integration test is still failing. It should succeed once https://github.com/cloudfoundry/capi-release/releases/tag/1.229.0 is used (last run was with 1.228.0). |
| var instanceDetails []ProcessInstance | ||
| if process.EmbeddedProcessInstances != nil { | ||
| for _, instance := range *process.EmbeddedProcessInstances { | ||
| instanceDetails = append(instanceDetails, NewProcessInstance(instance.Index, constant.ProcessInstanceState(instance.State), time.Duration(instance.Since))) |
There was a problem hiding this comment.
time.Duration(instance.Since) treats Since (an integer of seconds) as nanoseconds, not seconds. time.Duration's base unit is nanoseconds, so time.Duration(300) = 300ns, not 300s.
For comparison, the existing /stats endpoint unmarshal in process_instance.go correctly converts seconds via:
instance.Uptime, err = time.ParseDuration(fmt.Sprintf("%ds", inputInstance.Uptime))This means uptime values from the new path will be off by a factor of 10⁹, which will display effectively as zero uptime for all instances.
Fix:
instanceDetails = append(instanceDetails, NewProcessInstance(
instance.Index,
constant.ProcessInstanceState(instance.State),
time.Duration(instance.Since) * time.Second,
))The test expectations (e.g. Uptime: 300) also need updating to Uptime: 300 * time.Second to match the corrected behavior and the existing /stats code path.
| DiskInMB types.NullUint64 | ||
| LogRateLimitInBPS types.NullInt | ||
| AppGUID string | ||
| EmbeddedProcessInstances *[]EmbeddedProcessInstance |
There was a problem hiding this comment.
(nit - feel free to ignore)
Using *[]EmbeddedProcessInstance (pointer to slice) is unusual in Go — a nil slice already distinguishes "not present" from "present but empty". Consider whether []EmbeddedProcessInstance with omitempty (I think you already have this) on the JSON tag achieves the same semantics more idiomatically.
If the intent is to distinguish between "field absent from JSON" and "field present as []", then the pointer-to-slice is correct. But if that distinction isn't needed, a plain slice would be simpler and avoid the dereference (*process.EmbeddedProcessInstances) in consuming code.
Description of the Change
Use the new "embedded process instances" cloud controller feature to speed up the
cf appscommand, in particular on spaces with a larger number of apps. Instead of calling the individual/statsendpoint per process, use this request once per space:GET /v3/processes?space_guids=:guid&embed=process_instancesWhy Is This PR Valuable?
Time comparison on a test space with 100 apps and 2 instances each:
Applicable Issues
cloudfoundry/cloud_controller_ng#4796
How Urgent Is The Change?
Not super-urgent.