Skip to content

Conversation

adamint
Copy link
Member

@adamint adamint commented Jul 15, 2024

Currently, we are grouping applications based on whether their display names are identical, instead of using the ExecutableReplicaSet property from DCP.

The approach I took was

  1. Flow owner information, which includes the above property, from DCP through to the dashboard frontend
  2. Modify the 'application grouping' view model creation logic to use the real replica set for a resource
  3. Replace (replica set) with (application) on traces/logs resource dropdowns, because the OTLP applications there can be grouped, but are not replica sets.
    image
  4. Add a test around behavior for console logs view model creation (Unit Tests for Resource Name display text #1411)
     
    Fixes Unit Tests for Resource Name display text #1411
    Fixes Different instances of same resource type grouped as replicas in dashboard #4103
    Fixes Identify replica sets for dashboard #1233
Microsoft Reviewers: Open in CodeFlow

@adamint adamint requested a review from mitchdenny as a code owner July 15, 2024 20:00
@ghost ghost added the area-dashboard label Jul 15, 2024
@adamint
Copy link
Member Author

adamint commented Jul 15, 2024

@JamesNK Could you advise on what to do about ResourceOutgoingPeerResolverTests.NameAndDisplayNameDifferent_MultipleInstances_ReturnName - does the previous behavior of ResourceViewModel.GetResourceName need to stay the same for containers?

Comment on lines 676 to 682
private static ImmutableArray<OwnerReferenceSnapshot> GetOwners(CustomResource resource)
{
return [
..resource.Metadata.OwnerReferences
.Select(ownerReference => new OwnerReferenceSnapshot(ownerReference.Kind, ownerReference.Name, ownerReference.Uid))
];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd for something to have multiple owners. What are scenarios that would cause multiple owners?

cc @karolz-ms

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a K8s API concept and probably not the best name: it is a list of objects that depend on the given object. https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/object-meta/#System Most common scenario is when there is some sort of N:M relationship that is modeled as a separate object, the motivation for multiple owners being keeping the model consistent and cascade deletion.

For example (we are not doing this today, but we might in future): an Endpoint is exposed on a Network, implements a Service, and is implemented by an instance of a Container or Executable. The Network, Service, and implementing Container/Executable could all be "owner" objects.

@adamint adamint requested review from JamesNK and karolz-ms July 16, 2024 14:35
Adam Ratzman added 2 commits July 16, 2024 12:18
# Conflicts:
#	src/Aspire.Dashboard/wwwroot/css/app.css
# Conflicts:
#	src/Aspire.Dashboard/Resources/ControlsStrings.resx
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the changes in this PR except everything to do with getting and returning Owner info to the dashboard. It doesn't fix anything, it makes things more complicated, and it adds fields to the proto contract that we should think careful about before changing.

Think Owner changes should be removed from this PR.

@dotnet-policy-service dotnet-policy-service bot added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jul 19, 2024
@adamint adamint requested a review from JamesNK July 23, 2024 15:47
@drewnoakes drewnoakes mentioned this pull request Jul 24, 2024
6 tasks
@adamint adamint enabled auto-merge (squash) July 30, 2024 00:25
@adamint adamint merged commit 42ca143 into dotnet:main Jul 30, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants