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

test: load testing tooling and initial observations #225

Merged
merged 11 commits into from
Dec 13, 2022
Merged

test: load testing tooling and initial observations #225

merged 11 commits into from
Dec 13, 2022

Conversation

Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan commented Nov 25, 2022 •
edited

This PR

PR introduce tools to support performance testing for flagd. Content include,

  • A random feature flag generator
  • Sample K6 script
  • go pprof enabled flagd build with a dedicated Dockerfile (profile.Dockerfile)
  • Initial performance observations

Related Issues

PR is related to #212

How to test

Consider the readme contents at /test/loadtest/README.MD

@Kavindu-Dodan
init testing
07f833b 
Signed-off-by: Kavindu Dodanduwa <KavinduDodanduwa@gmail.com>
@Kavindu-Dodan
cleanup
43574a6 
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
tcarrio
tcarrio reviewed Nov 28, 2022
View reviewed changes
loadtest/ff_gen.go Outdated Show resolved Hide resolved
beeme1mr
beeme1mr reviewed Nov 28, 2022
View reviewed changes
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

How was the loadTestResults.png created?

@Kavindu-Dodan
review correction
dcf222f 
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Contributor Author

How was the loadTestResults.png created?

This is an extraction from the load test results [1]. If you are interested in how numbers were derived, it was by running a containerised flagd with random FFs (generated with ff_gen.go) and using the K6 script (included in the PR). Final result is the average of 3 test runs. And there was a warm-up time provided to flagd.

[1] https://docs.google.com/spreadsheets/d/1YLtkA-WryJXaZ4GWrbKhT_Izx3dh4spS/edit?usp=sharing&ouid=109303231969053490300&rtpof=true&sd=true

@beeme1mr beeme1mr self-requested a review November 28, 2022 15:53
@toddbaert
Copy link
Member

toddbaert commented Nov 28, 2022 •
edited

I think you should note somewhere that HTTP is used as the interface. Based on what I'm seeing, I think encoding and (de)serialization is probably where we are spending a lot of time. I think it would be ideal to run a suite in gRPC as well, using https://github.com/grpc/grpc/blob/master/doc/command_line_tool.md or https://github.com/fullstorydev/grpcurl, potentially. If we do this we'll want to make sure the same connection is used for all the RPCs. Do you think we should create an issue for this? Should we do it as part of this issue?

cc @beeme1mr

@beeme1mr
Copy link
Member

It would be worth extending the tests to also include gRPC. It looks like K6 supports it natively.

@Kavindu-Dodan
Specify API type tests were run against
3a4a2d4 
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Contributor Author

Kavindu-Dodan commented Nov 28, 2022 •
edited

I think you should note somewhere that HTTP is used as the interface. Based on what I'm seeing, I think encoding and (de)serialization is probably where we are spending a lot of time. I think it would be ideal to run a suite in gRPC as well, using https://github.com/grpc/grpc/blob/master/doc/command_line_tool.md or https://github.com/fullstorydev/grpcurl, potentially. If we do this we'll want to make sure the same connection is used for all the RPCs. Do you think we should create an issue for this? Should we do it as part of this issue?

cc @beeme1mr

Great input. I highlighted the API type used in the observations section (updated with latest commit).

Yes, testing and comparing against gRPC would be great. And yes, pprof proved the same point: most of the cpu time and memory is spent on reading and writing request

AlexsJones
AlexsJones reviewed Nov 29, 2022
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
loadtest/README.MD Outdated Show resolved Hide resolved
loadtest/README.MD Outdated Show resolved Hide resolved
skyerus
skyerus reviewed Nov 29, 2022
View reviewed changes
loadtest/README.MD Outdated Show resolved Hide resolved
Kavindu-Dodan and others added 2 commits November 29, 2022 08:05
@Kavindu-Dodan @skyerus
Update loadtest/README.MD
13ebd47 
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
@Kavindu-Dodan
New readme content and restructure tests
59bc037 
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@beeme1mr beeme1mr mentioned this pull request Nov 30, 2022
Closed
@beeme1mr
Copy link
Member

beeme1mr commented Dec 1, 2022

@Kavindu-Dodan please resolve any conversations that have been addressed.

@beeme1mr beeme1mr changed the title feat: load testing tooling and initial observations test: load testing tooling and initial observations Dec 1, 2022
@Kavindu-Dodan
Copy link
Contributor Author

@Kavindu-Dodan please resolve any conversations that have been addressed.

ACK. There's one more conversation to resolve and I am waiting for the response from @AlexsJones .

@AlexsJones
Copy link
Member

@Kavindu-Dodan please resolve any conversations that have been addressed.

ACK. There's one more conversation to resolve and I am waiting for the response from @AlexsJones .

Sorry just remarked on it

@Kavindu-Dodan
Copy link
Contributor Author

@Kavindu-Dodan please resolve any conversations that have been addressed.

ACK. There's one more conversation to resolve and I am waiting for the response from @AlexsJones .

Sorry just remarked on it

Thanks and I updated the PR with the suggested change

AlexsJones
AlexsJones requested changes Dec 7, 2022
View reviewed changes
profiler.go Show resolved Hide resolved
@skyerus skyerus mentioned this pull request Dec 13, 2022
Merged
@beeme1mr beeme1mr merged commit 26de63e into open-feature:main Dec 13, 2022
@Kavindu-Dodan Kavindu-Dodan deleted the feat/load-test-init branch December 13, 2022 17:52
@toddbaert toddbaert mentioned this pull request Feb 6, 2023
Merged
beeme1mr pushed a commit that referenced this pull request Feb 7, 2023
@toddbaert
chore: add @Kavindu-Dodan to CODEOWNERS (#377)
e02c899 
@Kavindu-Dodan has contributed multiple significant changes and
proposals to flagd:

- multiple refactors: #291,
#307
- ci/security improvements:
#338,
#337
- architectural proposals (some of which got some attention from outside
parties!): open-feature/ofep#45,
open-feature/flagd-schemas#78,
#249 (comment)
- load testing: #225
- documentation improvements

For these reasons, I believe he should be made a CODEOWNER in this
repository.

NOTE: before this is merged, @Kavindu-Dodan should be added with at
least `maintainer` permissions to the repo.

Signed-off-by: Todd Baert <toddbaert@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcarrio tcarrio tcarrio left review comments

@james-milligan james-milligan james-milligan left review comments

@beeme1mr beeme1mr beeme1mr approved these changes

@skyerus skyerus skyerus approved these changes

@AlexsJones AlexsJones Awaiting requested review from AlexsJones

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Kavindu-Dodan @toddbaert @beeme1mr @AlexsJones @tcarrio @skyerus @james-milligan