Skip to content

Add instance_guid field in Process Stats response #2831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 7, 2025

Conversation

tcdowney
Copy link
Member

@tcdowney tcdowney commented Jun 9, 2022

A short explanation of the proposed change:

  • Display the Diego instance identifier as part of the Process Stats
    response. This id is unique for each actual LRP instance and can be used
    as a substitute for the integer index id.
  • Eirini does not support this in their API at this time, so the OPI
    variant of the instances stats reporter returns an empty string for this
    field. In the future, if Eirini is enhanced to provide this we can
    update that client.
  • Issue: Feature Request: Include LRP instance_id in Process Stats response #2819

An explanation of the use cases your change solves:

The issue (#2819) describes this use case in more detail. The summary though is this can help developers debug their apps (this id is logged by app containers) and provide a pathway for clients to provide other container ids than just the integer index.

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@tcdowney
Copy link
Member Author

@Gerg some additional context for this change:

On Korifi we're wanting to support workloads other than StatefulSets (e.g. Deployments, knative Services, etc.) so we're working toward reducing the reliance of system components on the numerical indexing of app instances. I'll be putting out a proposal / some sort of design doc to start discussing this in more detail later this week, but I see this change as a first step toward that.

@tcdowney tcdowney marked this pull request as draft June 23, 2022 23:12
@tcdowney
Copy link
Member Author

I'm putting this into draft state. I was looking at the AppRecipeBuilder and noticed that instance_id is that metric tag we're adding and it corresponds to instance index. We are calling this the process_instance_id on metrics/logs so maybe that name should be used. Worth discussing I think.

'process_instance_id' => METRIC_TAG_VALUE.new(dynamic: METRIC_TAG_VALUE::DynamicValue::INSTANCE_GUID),

@Gerg
Copy link
Member

Gerg commented Jun 29, 2022

It's a little odd that we call index instance_id in the metrics tags. Looks like it came from this story.

index/guid is the most intuitive to me, but I could see other options. Process.instance.process_instance_id sounds a bit redundant.

@tcdowney
Copy link
Member Author

@Gerg

Yeah, I started calling it instance_guid here in the proposal around this stuff here:
https://docs.google.com/document/d/118yYN4WGj1F2si8gk94xFZsYdewtOKoqO2Dv-1I-PgE/edit

@Gerg
Copy link
Member

Gerg commented Jun 29, 2022

What about just guid?

@tcdowney tcdowney closed this Aug 21, 2023
@tcdowney tcdowney reopened this Jan 14, 2025
@tcdowney tcdowney force-pushed the instance-id-on-process-stats-issue-2819 branch from f701464 to 5d7ac94 Compare March 26, 2025 21:38
@tcdowney tcdowney marked this pull request as ready for review March 26, 2025 21:38
@tcdowney tcdowney requested review from Samze and removed request for tjvman March 26, 2025 21:38
@tcdowney
Copy link
Member Author

cc @zrob

@tcdowney tcdowney force-pushed the instance-id-on-process-stats-issue-2819 branch from f87a6a9 to 519b8ce Compare March 26, 2025 21:47
@tcdowney tcdowney changed the title Add instance_id field in Process Stats response Add instance_guid field in Process Stats response Mar 26, 2025
@tcdowney
Copy link
Member Author

What about just guid?

I went with instance_guid. There are other fields prefixed with instance_ and it felt less ambiguous since this is what the field is called on the ActualLRP.
https://github.com/cloudfoundry/bbs/blob/main/docs/030-lrps.md#instance_guid

- Display the Diego instance identifier as part of the Process Stats
response. This id is unique for each actual LRP instance and can be used
as a substitute for the integer index id.
- Issue: #2819
@tcdowney tcdowney force-pushed the instance-id-on-process-stats-issue-2819 branch from 519b8ce to 1a39571 Compare March 26, 2025 22:46
@tcdowney
Copy link
Member Author

The docs failure is unrelated to my change. It looks like some of the npm packages we're using for the doc link checker are not working with modern node.js? I don't have much experience with all that so not sure, but I feel like we can ignore this for this PR.

@tcdowney tcdowney merged commit f50766e into main Apr 7, 2025
8 of 9 checks passed
@tcdowney tcdowney deleted the instance-id-on-process-stats-issue-2819 branch April 7, 2025 20:35
ari-wg-gitbot added a commit to cloudfoundry/capi-release that referenced this pull request Apr 7, 2025
Changes in cloud_controller_ng:

- Add instance_guid field in Process Stats response
    PR: cloudfoundry/cloud_controller_ng#2831
    Author: Tim Downey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants