-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Specify additional specific load generator options in mesheryctl perf #8148
Specify additional specific load generator options in mesheryctl perf #8148
Conversation
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also provide a sample config? More better a recording which showcases additional options being used.
server/handlers/load_test_handler.go
Outdated
performanceProfileData, err := provider.GetPerformanceProfile(req, profileID) | ||
if err != nil { | ||
h.log.Error(err) | ||
http.Error(w, ErrLoadCertificate(err).Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please return correct error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdullah1308 does this make sense?
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
@leecalcote @MUzairS15 resolved suggestions |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #8148 +/- ##
=========================================
- Coverage 5.22% 5.21% -0.01%
=========================================
Files 124 124
Lines 17522 17628 +106
=========================================
+ Hits 915 919 +4
- Misses 16435 16537 +102
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@abdullah1308 A working demo will be helpful via mesheryctl indicating additional falgs were used. |
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
@abdullah1308 run P.S: I know I didn't include the CI run for that but I disabled that to avoid several pushes by l5io bot |
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
@alphaX86 unable to run it now :/
|
Related to #8203 which @luigidematteis is working on. |
mesheryctl perf apply meshery-profile --url "https://google.com" --useCert | ||
|
||
// Execute a Performance test creating a new performance profile and pass certificate to be used | ||
mesheryctl perf apply meshery-profile-new --url "https://google.com" --certPath path/to/cert.pem --useCert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the one flag --certPath
is enough, what is the purpose of --useCert
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gyohuangxin Currently the API supports running a test with/without the certificate contained in a performance profile. --useCert
is a boolean flag to toggle this behaviour.
--certPath
is used to add a certificate to a new profile being created. A certificate can be added to a test profile only while creating it. Currently server-side there isn't a way to include a certificate for a single run.
I was initially considering having only one flag that does both of the above behaviours. But the CLI library doesn't permit a flag to have both a boolean behaviour and an argument behaviour. Hence I've broken it down into 2 flags.
Hence
mesheryctl perf apply meshery-profile-new --url "https://google.com" --certPath path/to/cert.pem --useCert
causes a new profile to be created with the specified certificate and --useCert
causes the certificate to be used when the test is run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood about adding "--useCert" to allow the user to choose whether to use a certificate in the performance profile. But can we replace it with "--disableCert" and set its default value to false? Because I think it may be confusing that I need to add --useCert
when I have --certPath
set. If we use --disableCert
, we don't need to explicitly add another flag and keep the command concise, like
mesheryctl perf apply meshery-profile-new --url "https://google.com" --certPath path/to/cert.pem
If someone doesn't want to use the certificates in a proformance profile, then he can use --disableCert
mesheryctl perf apply meshery-profile --url "https://google.com" --disableCert
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gyohuangxin I like the idea having certificates being used by default. I'll change the flag to --disableCert
.
a couple of merge conflicts popped up. |
@abdullah1308 Let's discuss this on Meshery Dev call. Please add this as an agenda item in the meeting minutes if you would. :) |
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Command Run (Fortio)
Debug Log
Command Run (Wrk2)
Debug Log
Unable to demo example for nighthawk as I'm on MacOS. |
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Latest help docs
|
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
I wonder if we might toss the set of available load generator-specific flags into https://docs.meshery.io/tasks/performance-management |
❗ nighthawk doesn't work |
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Nighthawk works ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Ready to be merged? |
@abdullah1308 What’s holding this? |
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
@MUzairS15 @gyohuangxin This PR should be ready for merge. |
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Signed-off-by: abdullah1308 <abdullahrafi.1308@gmail.com>
Notes for Reviewers
This PR fixes #8047
Signed commits