Skip to content

Add instance_guid field in Process Stats response#2831

Merged
tcdowney merged 2 commits intomainfrom
instance-id-on-process-stats-issue-2819
Apr 7, 2025
Merged

Add instance_guid field in Process Stats response#2831
tcdowney merged 2 commits intomainfrom
instance-id-on-process-stats-issue-2819

Conversation

@tcdowney
Copy link
Copy Markdown
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 tcdowney requested review from Gerg, sethboyles and tjvman June 21, 2022 17:22
@tcdowney
Copy link
Copy Markdown
Member Author

tcdowney commented Jun 21, 2022

@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
Copy Markdown
Member Author

tcdowney commented Jun 23, 2022

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.

cloud_controller_ng/lib/cloud_controller/diego/app_recipe_builder.rb

Line 108 in 7698bc4

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

@Gerg
Copy link
Copy Markdown
Member

Gerg commented Jun 29, 2022 •
edited
Loading

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
Copy Markdown
Member Author

tcdowney commented Jun 29, 2022

@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
Copy Markdown
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
Copy Markdown
Member Author

tcdowney commented Mar 26, 2025

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
Copy Markdown
Member Author

tcdowney commented Mar 26, 2025

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

tcdowney added 2 commits March 26, 2025 16:14
@tcdowney
Add instance_id field in Process Stats response
f7d8d6c 
- 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
Rename Process stats instance_id to instance_guid
1a39571
@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
Copy Markdown
Member Author

tcdowney commented Mar 27, 2025

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
@ari-wg-gitbot
Bump cloud_controller_ng
dc7a5f4 
Changes in cloud_controller_ng:

- Add instance_guid field in Process Stats response
    PR: cloudfoundry/cloud_controller_ng#2831
    Author: Tim Downey <tcdowney@users.noreply.github.com>
@tcdowney tcdowney mentioned this pull request Jun 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Samze Samze Samze approved these changes

@Gerg Gerg Gerg approved these changes

@sethboyles sethboyles sethboyles approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tcdowney @Gerg @Samze @sethboyles @cf-gitbot