Skip to content
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

feat: add constructor for full event lineage graph #7921

Merged
merged 12 commits into from
Sep 18, 2024

Conversation

Cali0707
Copy link
Member

Fixes #7694

Proposed Changes

  • Get resources from cluster and construct a graph

Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 13, 2024
@Cali0707
Copy link
Member Author

/cc @pierDipi

@knative-prow knative-prow bot requested a review from pierDipi May 13, 2024 18:41
Copy link

knative-prow bot commented May 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot requested review from aslom and matzew May 13, 2024 18:41
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2024
pkg/graph/constructor.go Outdated Show resolved Hide resolved
@Cali0707
Copy link
Member Author

This is WIP because I want feedback on the approach before adding unit tests

Signed-off-by: Calum Murray <cmurray@redhat.com>
Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 9.67742% with 224 lines in your changes missing coverage. Please review.

Project coverage is 67.04%. Comparing base (2cc0a5b) to head (d60a069).
Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
pkg/graph/constructor.go 10.90% 196 Missing ⚠️
pkg/graph/types.go 0.00% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7921      +/-   ##
==========================================
- Coverage   67.87%   67.04%   -0.83%     
==========================================
  Files         368      368              
  Lines       17551    17777     +226     
==========================================
+ Hits        11912    11918       +6     
- Misses       4894     5113     +219     
- Partials      745      746       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/graph/constructor.go Outdated Show resolved Hide resolved
pkg/graph/constructor.go Outdated Show resolved Hide resolved
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2024
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2024
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

/cc @pierDipi @aliok

@knative-prow knative-prow bot requested review from aliok and pierDipi July 23, 2024 03:51
pkg/graph/constructor.go Outdated Show resolved Hide resolved
Signed-off-by: Calum Murray <cmurray@redhat.com>
pkg/graph/constructor.go Outdated Show resolved Hide resolved
pkg/graph/constructor.go Outdated Show resolved Hide resolved

return g, nil
}

func (g *Graph) AddBroker(broker eventingv1.Broker) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we actually embed the full broker resource in the graph here? Or make it a config option?

Because, for example in the Backstage backend, if I only have references, I will need to fetch the brokers again. But, we have the full resource here fetched anyway.

Copy link
Member

Choose a reason for hiding this comment

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

@Cali0707 this is done, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the resources are all added with type interface{} to each vertex

@aliok
Copy link
Member

aliok commented Jul 25, 2024

Kind of irrelevant for this PR, but it would be super nice if we have a stringer for graph and vertices for debugging purposes. A text like

A (broker.eventing.knative.dev) --> B (foo.bar)
...etc

@Cali0707
Copy link
Member Author

/cc @aliok
Thanks for all the feedback!!

@knative-prow knative-prow bot requested a review from aliok July 25, 2024 18:43
@Cali0707
Copy link
Member Author

Kind of irrelevant for this PR, but it would be super nice if we have a stringer for graph and vertices for debugging purposes. A text like

A (broker.eventing.knative.dev) --> B (foo.bar)
...etc

Yeah 100%, I thought about this too but I wasn't sure how to show a vertex with an arbitrary number of edges using just strings...

@aliok
Copy link
Member

aliok commented Jul 26, 2024

Yeah 100%, I thought about this too but I wasn't sure how to show a vertex with an arbitrary number of edges using just strings...

I added stringer impls for types now. It is ok-ish. Example:

Kind = Broker, Namespace = default, Name = test-broker, APIVersion = eventing.knative.dev/v1, Group = , Address = 
Kind = Broker, Namespace = default, Name = default, APIVersion = eventing.knative.dev/v1, Group = , Address = 
Kind = EventType, Namespace = default, Name = test-event-type, APIVersion = eventing.knative.dev/v1beta3, Group = , Address = 
Out Edges:
        [Kind = EventType, Namespace = default, Name = test-event-type, APIVersion = eventing.knative.dev/v1beta3, Group = , Address = ] --> [Kind = Broker, Namespace = default, Name = test-broker, APIVersion = eventing.knative.dev/v1, Group = , Address = ]

@aliok
Copy link
Member

aliok commented Jul 26, 2024

@Cali0707

Say we have an EventType:

kind: EventType
metadata:
  name: test-event-type
  namespace: default
spec:
  reference:
    apiVersion: eventing.knative.dev/v1
    kind: Broker
    name: test-broker

Currently the graph is constructed in a way that there is an edge from ET to Broker.

I think it should be the other way around.

Events of EventType is coming out of broker.

cc @pierDipi

aliok and others added 5 commits July 29, 2024 15:04
for 3 entries, it was creating an array of 6

Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
@Cali0707
Copy link
Member Author

@aliok @pierDipi is there anything left for this PR? Should we merge this and move to some follow up issues/PRs for anything that comes up?

@Cali0707
Copy link
Member Author

@aliok do you think we can merge this PR and then make any required changes on follow up PRs?

@aliok
Copy link
Member

aliok commented Aug 19, 2024

/test reconciler-tests

@aliok
Copy link
Member

aliok commented Aug 19, 2024

I think we can merge this one.

I can make changes later on

@aliok
Copy link
Member

aliok commented Aug 20, 2024

/test reconciler-tests

@Cali0707
Copy link
Member Author

/retest-required

@Cali0707 Cali0707 changed the title [WIP] feat: add constructor for full event lineage graph feat: add constructor for full event lineage graph Aug 21, 2024
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2024
@Cali0707
Copy link
Member Author

/cc @aliok @pierDipi @creydr can we move forwards with this PR?

@pierDipi pierDipi requested a review from aliok September 16, 2024 15:28
@aliok
Copy link
Member

aliok commented Sep 17, 2024

/lgtm
/hold

Holding for others to review if necessary

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2024
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2024
@pierDipi
Copy link
Member

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2024
@knative-prow knative-prow bot merged commit b48b664 into knative:main Sep 18, 2024
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Event Lineage: Create Graph from Cluster Resources
4 participants