Skip to content

Specify files for port detection for every detector#28

Merged
thepetk merged 31 commits intodevfile:mainfrom
thepetk:1151_ft/specific_port_detection
Sep 22, 2023
Merged

Specify files for port detection for every detector#28
thepetk merged 31 commits intodevfile:mainfrom
thepetk:1151_ft/specific_port_detection

Conversation

@thepetk
Copy link
Contributor

@thepetk thepetk commented Sep 19, 2023 •
edited
Loading

What does this PR do?

This PR introduces new functionality for every detector in order to ensure a consistent way of picking files for port detection. It also adds more comments, the required unit tests and updates to the documentation.

Updates

[Consistency]

  • The ApplicationFileInfo model is expanded to contain the component path and the context. That way it can be the main model used to identify files that may contain information about port detection.

  • In every detector we add the GetApplicationFileInfos which will return specific ApplicationFileInfo for every framework.

[Performance]

  • For detectors which are looking inside the entire component to find source code files in order detect ports, the GenerateApplicationFileFromFilters func has been created. This way we are passing a search keyword in order to go through specific files. For example, we are passing ".go" for all golang frameworks as, for port detection, we are interested only for golang files.

[Other Updates]

  • As each detector has different functionality for port detection, the GetApplicationFileContents and GetApplicationFileBytes in order to fetch the content of each application file as string or bytes.
  • All models types from detector have been moved to model.go and sorted by name. This way we have a specific place to store all model's code and not have them spread around different modules.
  • Further refactoring has been made to flaskdetector and djangodetector to ensure that we are looping through application files and not picking them one by one.

Results

  1. Performance wise, for a big project with many components (e.g. https://github.com/openshift/hypershift) we can see big difference in the execution time. The main reason is that for many detectors we are filtering the files that we are parsing in order to detect ports. The results:
# old
$ time ./alizer component ~/github/alizer-tests/hypershift/
...
real	0m4,043s
user	0m4,281s
sys	0m0,368s

# new
time ./alizer component ~/github/alizer-tests/hypershift/
...
real	0m2,556s
user	0m2,909s
sys	0m0,318s

  1. With the new way of defining potential application files we need only to add an item to every detector's GetApplicationFileInfos and not update the whole DoPortDetection func.

  2. Ensuring that all models are stored in the same place and they have detailed naming. For example, the ServerXml from OpenLibertyDetector has been moved to model.go and renamed to OpenLibertyServerXml, in order to be easier identified and found.

Which issue(s) does this PR fix

fixes devfile/api#1151

PR acceptance criteria

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.

  • Unit/Functional tests

  • Documentation

How to test changes / Special notes to the reviewer

@thepetk thepetk self-assigned this Sep 19, 2023
@openshift-ci openshift-ci bot requested a review from elsony September 19, 2023 12:48
@thepetk thepetk marked this pull request as draft September 19, 2023 12:49
@codecov
Copy link

codecov bot commented Sep 19, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.59% 🎉

Comparison is base (89027d1) 67.89% compared to head (c1d3f23) 70.48%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   67.89%   70.48%   +2.59%     
==========================================
  Files          11       11              
  Lines        1523     1555      +32     
==========================================
+ Hits         1034     1096      +62     
+ Misses        423      393      -30     
  Partials       66       66
Files Changed Coverage Δ
pkg/utils/detector.go 86.36% <100.00%> (+7.60%) ⬆️

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

feloy
feloy reviewed Sep 20, 2023
View reviewed changes
@thepetk thepetk changed the title Draft: Specify files for port detection for every detector Specify files for port detection for every detector Sep 20, 2023
@thepetk thepetk marked this pull request as ready for review September 20, 2023 12:45
@thepetk
Copy link
Contributor Author

thepetk commented Sep 20, 2023

Updates on commenting & documentation have been added. The draft status is now removed

@thepetk thepetk requested a review from feloy September 20, 2023 12:51
@thepetk thepetk force-pushed the 1151_ft/specific_port_detection branch from ba40f40 to 67644fa Compare September 21, 2023 11:36
rm3l
rm3l reviewed Sep 21, 2023
View reviewed changes
@feloy feloy mentioned this pull request Sep 21, 2023
Closed
rm3l
rm3l reviewed Sep 21, 2023
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I was just benchmarking the performance improvements locally (using the same hypershift repo) and noticed a strange behavior.
When running from the Alizer source code:

☸ ~/w/p/d/alizer on  1151_ft/specific_port_detection [$!?] via 🐹 v1.19.12 
$ go build -o alizer alizer.go

☸ ~/w/p/d/alizer on  1151_ft/specific_port_detection [$!?] via 🐹 v1.19.12 
$ time ./alizer component /home/asoro/work/tmp/hypershift
[...]
./alizer component /home/asoro/work/tmp/hypershift  18.74s user 1.82s system 123% cpu 16.644 total

But when running the same Alizer binary from the hypershift repo directory, it takes very long:

☸ ~/w/t/hypershift on  main via 🐹 v1.21.0 
$ time /home/asoro/work/projects/devfile/alizer/alizer component .
[...]
/home/asoro/work/projects/devfile/alizer/alizer component .  142.18s user 3.05s system 111% cpu 2:10.65 total

Do you have the same issue? Or am I missing something?

@thepetk
Update dotnet detector with universal port detection
3f93eef 
Signed-off-by: thepetk <[email protected]>
@thepetk
Align go detectors to universal port detection
3f4de79 
Signed-off-by: thepetk <[email protected]>
@thepetk
TMP update detector
6dcd5d6 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update various java frameworks
6576339 
Signed-off-by: thepetk <[email protected]>
@thepetk
Finalize quarkus port detection
bcb6fb9 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update spring port detection
c9acc73 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update Vertx port detection
900d32c 
Signed-off-by: thepetk <[email protected]>
@thepetk
Use consistent naming for enricher models
c51fcfa 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update wildfly port detection
3a15b15 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update angular port detection
491d05d 
Signed-off-by: thepetk <[email protected]>
@thepetk
Add filtering to js files for express enricher
ba30bfd 
Signed-off-by: thepetk <[email protected]>
thepetk and others added 19 commits September 21, 2023 13:30
@thepetk
Update nuxt port detection
2b1614d 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update react port detection
c3b61f7 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update svelte js port detection
7e2b15f 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update vue js port detection
10266d1 
Signed-off-by: thepetk <[email protected]>
@thepetk
Sort models
7f469e5 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update laravel port detection
9ab7eaf 
Signed-off-by: thepetk <[email protected]>
@thepetk
Refactor django enricher
6479e42 
Signed-off-by: thepetk <[email protected]>
@thepetk
Loop for every application file found during port detection process
8e0aaf6 
Signed-off-by: thepetk <[email protected]>
@thepetk
Refactor flask enricher
0431b18 
Signed-off-by: thepetk <[email protected]>
@thepetk
Fix vertx port detection
8859bec 
Signed-off-by: thepetk <[email protected]>
@thepetk
Remove unused model
51ee65e 
Signed-off-by: thepetk <[email protected]>
@thepetk
Add comments to all func in detector.go
0aac45e 
Signed-off-by: thepetk <[email protected]>
@thepetk
Improve detector_test coverage
0ab539e 
Signed-off-by: thepetk <[email protected]>
@thepetk
Add comments to model.go
af006a7 
Signed-off-by: thepetk <[email protected]>
@thepetk
Update documentation for port detection
c80b7fd 
Signed-off-by: thepetk <[email protected]>
@thepetk @rm3l
Update docs/public/port_detection.md
f830f56 
Signed-off-by: thepetk <[email protected]>

Co-authored-by: Armel Soro <[email protected]>
Signed-off-by: thepetk <[email protected]>
@thepetk @rm3l
Update docs/public/port_detection.md
4d61557 
Signed-off-by: thepetk <[email protected]>

Co-authored-by: Armel Soro <[email protected]>
Signed-off-by: thepetk <[email protected]>
@thepetk @rm3l
Update docs/public/port_detection.md
d5e253c 
Signed-off-by: thepetk <[email protected]>

Co-authored-by: Armel Soro <[email protected]>
Signed-off-by: thepetk <[email protected]>
@thepetk
Return nil on not implemented GetApplicationFileInfos
c1d3f23 
Signed-off-by: thepetk <[email protected]>
@thepetk thepetk force-pushed the 1151_ft/specific_port_detection branch from 2c1124f to c1d3f23 Compare September 21, 2023 12:30
@thepetk
Copy link
Contributor Author

thepetk commented Sep 21, 2023

I was just benchmarking the performance improvements locally (using the same hypershift repo) and noticed a strange behavior. When running from the Alizer source code:

☸ ~/w/p/d/alizer on  1151_ft/specific_port_detection [$!?] via 🐹 v1.19.12 
$ go build -o alizer alizer.go

☸ ~/w/p/d/alizer on  1151_ft/specific_port_detection [$!?] via 🐹 v1.19.12 
$ time ./alizer component /home/asoro/work/tmp/hypershift
[...]
./alizer component /home/asoro/work/tmp/hypershift  18.74s user 1.82s system 123% cpu 16.644 total

But when running the same Alizer binary from the hypershift repo directory, it takes very long:

☸ ~/w/t/hypershift on  main via 🐹 v1.21.0 
$ time /home/asoro/work/projects/devfile/alizer/alizer component .
[...]
/home/asoro/work/projects/devfile/alizer/alizer component .  142.18s user 3.05s system 111% cpu 2:10.65 total

Do you have the same issue? Or am I missing something?

@rm3l no locally I have ~5secs if I run it from the directory of Hypershift and ~3secs if I run it from alizer dir

@rm3l
Copy link
Member

rm3l commented Sep 21, 2023

I was just benchmarking the performance improvements locally (using the same hypershift repo) and noticed a strange behavior. When running from the Alizer source code:

☸ ~/w/p/d/alizer on  1151_ft/specific_port_detection [$!?] via 🐹 v1.19.12 
$ go build -o alizer alizer.go

☸ ~/w/p/d/alizer on  1151_ft/specific_port_detection [$!?] via 🐹 v1.19.12 
$ time ./alizer component /home/asoro/work/tmp/hypershift
[...]
./alizer component /home/asoro/work/tmp/hypershift  18.74s user 1.82s system 123% cpu 16.644 total

But when running the same Alizer binary from the hypershift repo directory, it takes very long:

☸ ~/w/t/hypershift on  main via 🐹 v1.21.0 
$ time /home/asoro/work/projects/devfile/alizer/alizer component .
[...]
/home/asoro/work/projects/devfile/alizer/alizer component .  142.18s user 3.05s system 111% cpu 2:10.65 total

Do you have the same issue? Or am I missing something?

@rm3l no locally I have ~5secs if I run it from the directory of Hypershift and ~3secs if I run it from alizer dir

Ok, never mind. Fine then if it is okay on your end. It looks like I am experiencing similar issues with other tools. Thanks for checking!

@thepetk
Copy link
Contributor Author

thepetk commented Sep 21, 2023

Ok, never mind. Fine then if it is okay on your end. It looks like I am experiencing similar issues with other tools. Thanks for checking!

np @rm3l! Feel free to ltgm :)

@thepetk thepetk requested a review from rm3l September 22, 2023 09:13
@openshift-ci openshift-ci bot added the lgtm label Sep 22, 2023
@openshift-ci
Copy link

openshift-ci bot commented Sep 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l, thepetk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@thepetk thepetk merged commit e486794 into devfile:main Sep 22, 2023
@thepetk thepetk deleted the 1151_ft/specific_port_detection branch November 9, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michael-valdron michael-valdron Awaiting requested review from michael-valdron

@elsony elsony Awaiting requested review from elsony

@feloy feloy Awaiting requested review from feloy

1 more reviewer

@rm3l rm3l rm3l approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rm3l rm3l

@thepetk thepetk

Labels

approved lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add specific port detection paths to each alizer detector

3 participants

@thepetk @rm3l @feloy