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.

Sign up for GitHub

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

Jump to bottom

#1526 Benchmark for startup when merging multiple config files #1856

Closed
wants to merge 5 commits into from
Closed

#1526 Benchmark for startup when merging multiple config files #1856

wants to merge 5 commits into from

Conversation

ks1990cn
Copy link
Contributor

@ks1990cn ks1990cn commented Dec 19, 2023 •
edited by raman-m
Loading

Fixes #1526

Adding benchmark for startup.
We can replicate issue or verify with this PR

Unable to replicate application down with 600 dummy routes.

Proposed Changes

  • Add HeavyRoutesStartupBenchmark benchmark
  • Current benchmarking is done for 600+ dummy routes across 2 files, more file and more dummy routes can be extended easily.

@raman-m
Copy link
Member

raman-m commented Dec 19, 2023

Wow! Tanmay!
I'm impressed!

But why do you create new PR(s) having ignored the work on other ones?
Seems you need to work on #1842 ... right?

I would say the team can review this PR after work completion of the previous PR #1842 .
Why not to focus on the one PR?

@ggnaegi FYI

@ks1990cn
Copy link
Contributor Author

@raman-m , I was waiting for @ggnaegi reply on PR. Meanwhile I started trying with different issues 😄

@raman-m raman-m added bug Identified as a potential bug Configuration Ocelot feature: Configuration labels Feb 17, 2024
@raman-m
Copy link
Member

raman-m commented Feb 17, 2024 •
edited
Loading

Proposed Changes

  • Current benchmarking is done for 600+ dummy routes across 2 files, more file and more dummy routes can be extended easily.

Tanmay, seems you didn't understand clearly the user scenario:

Previously I include seven different configurations files with the routes of different projects API, The problem occurred when a new file is added with more than 80 routes in it.

First, we don't know how many routes were defined in these old 7 files... maybe 100, but maybe 500, mauve only 7 😄
I asked the author to attach original config files, but she didn't care....

My understanding of #1526 is the following:

  • New file with 80 routes has different routes. In this case the problem can be clean validation error(s).
  • New 80 routes are overrides of old routes. In this case something happen in merging logic. But we cannot check this scenario because the author didn't attach original files.

Your benchmark is great! And seems a bit more development should be provided.

@ggnaegi Could you review this PR please, and advise on benchmark quality? I believe it is useful, but it needs some refactoring.

raman-m
raman-m requested changes Feb 17, 2024
View reviewed changes
test/Ocelot.Benchmarks/HeavyRoutesStartupBenchmark.cs
Comment on lines +30 to +32
//Setting up for more than 600 routes in different files
CreateOcelotConfigFile(0,100,"ocelot.json");
CreateOcelotConfigFile(101, 500, "ocelot.second.json");
Copy link
Member

Choose a reason for hiding this comment

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

1st step: We need to have some routes in 7+ files
2nd step: We add new file with 80+ routes

We need to split into 2 scenarios

  • new file routes are new ones
  • new file routes override routes in old files

test/Ocelot.Benchmarks/HeavyRoutesStartupBenchmark.cs Show resolved Hide resolved
test/Ocelot.Benchmarks/HeavyRoutesStartupBenchmark.cs
Comment on lines +66 to +79
private int FindAvailablePort()
{
TcpListener listener = null;
try
{
listener = new TcpListener(IPAddress.Loopback, 0);
listener.Start();
return ((IPEndPoint)listener.LocalEndpoint).Port;
}
finally
{
listener?.Stop();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Pretty new logic?
Why not to use helper from Ocelot.Testing project. But the PortFinder uses Socket to bind to.
You use TcpListener... So it can be a new helper in Ocelot.Testing project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

test/Ocelot.Benchmarks/HeavyRoutesStartupBenchmark.cs
public void Cleanup()
{
// Stop the web host after benchmarking
_ocelot.StopAsync().GetAwaiter().GetResult();
Copy link
Member

Choose a reason for hiding this comment

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

Why not await _ocelot.StopAsync();? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@raman-m raman-m changed the title Benchmark for startup #1526 Benchmark for startup when merging multiple config files Feb 17, 2024
@raman-m
Copy link
Member

raman-m commented Feb 17, 2024

⚠️ I've rebased the branch onto develop!

@ks1990cn ks1990cn closed this by deleting the head repository Feb 18, 2024
@ks1990cn
Copy link
Contributor Author

Sorry for inconvenience, I'll be raising PR again. Something went wrong with my develop branch and was getting unable to find latest commits on it.

@ks1990cn ks1990cn mentioned this pull request Feb 18, 2024
Draft
@raman-m
Copy link
Member

raman-m commented Feb 18, 2024

With develop or feature branch?

Please keep your develop identical to head repo develop!
Use the button Sync fork!

Regarding feature branch...
As I said I've rebased it onto develop!
What can be wrong here?
Your old local commits are not compatible to new commits on origin.
To fix that you have to re-check out the branch:

git switch develop
git branch -D feature
git fetch --all
git checkout feature

Clear?

@ks1990cn
Copy link
Contributor Author

I use same, sync branch sir. But somehow I was getting unnnecessary commits in that and I was ahead of 4-5 commits on develop.
On merging that to my this branch may have caused issue.

So I decided to refork!

@ks1990cn
Copy link
Contributor Author

I am sorry sir @raman-m .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raman-m raman-m raman-m requested changes

Assignees

@ks1990cn ks1990cn

Labels
bug Identified as a potential bug Configuration Ocelot feature: Configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to optimize ocelot startup time.
2 participants
@ks1990cn @raman-m