-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove grpc from loadgenerator #688
Remove grpc from loadgenerator #688
Conversation
Signed-off-by: svrnm <neumanns@cisco.com>
Signed-off-by: svrnm <neumanns@cisco.com>
Another big perf increase for me. 13 vs 174 seconds on m2 mac air. Please keep these coming lol |
Im getting some weird issues from the cart and java async service. The cart service is showing arch64 issues / confusion. I think the addition of that support in the currency service might be messing with things but not sure |
on a mac m2 |
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, there is just a trailing space in the changelog.
Signed-off-by: svrnm <neumanns@cisco.com>
TY, fixed.
odd, what errors exactly are you getting? I can try to reproduce it on a M1 |
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.
Seems to be running fine for me, also cart and currency services, when building locally on M1.
system prune did the trick. lgtm. I'm seeing feature flag service errors like mentioned in a recent issue but thats unrelated |
Hi. I am trying to understand the reasoning behind this PR? This change decreases build time and lowers image size, but I don't think we should optimize for that here. Maybe I am missing something, but I think load generator should generate gRPC load. Shouldn't demo project closely resemble real life usage? Yes, some opentelemetry users may be using OTLP over HTTP in production, but most of them should probably use OTLP over gRPC. In my opinion, if possible, loadgenerator should test both gRPC and HTTP transports |
@imomaliev thanks for sharing your perspective. There’s a couple reasons an optimized build time was desperately needed. Our previous releases were taking an hour plus to publish, we couldn’t publish for multiple chip architectures, additional services and features like the queue were only increasing performance issues, and contributor experience was suffering due to high local build times. GRPC was a huge culprit in all of that and selective removal from some services made a huge difference in build time. Our current version took 7 mins to build! In this case we chose use user experience over being perfectly life like. |
@cartersocha Thank you for the explanation. Developer ergonomics is a must for improving contributor's comfort, in turn making contributing a more welcoming experience. So I completely get the reasoning. Maybe we should at least provide examples and directions on how people may manually build loadgenerator with gRPC suite if they choose to? |
To be more specific on this issue with the LoadGenerator. gRPC was only used to send tracing data to the OpenTelemetry collector. The load generator generates the load against the frontend service, which only receives communication in HTTP form (it's a website). Removing it here made lots of sense because we saved on build time without sacrificing anything to do with the actual functionality of the load generator component. Internally all services but a select few communicate with each other using gRPC. I think the select few here are really just the email and quote services because gRPC support in Ruby and PHP is lacking. We are targeting to remove gRPC from the currency service because gRPC makes up a significant portion of the build time in C++, which drags on developer experience. You can see how each service communicates (HTTP, gRPC, TCP) in the service architecture diagram for the demo. |
* Remove grpc from loadgenerator Signed-off-by: svrnm <neumanns@cisco.com> * Update CHANGELOG.md Signed-off-by: svrnm <neumanns@cisco.com> * remove white space in changelog Signed-off-by: svrnm <neumanns@cisco.com> Signed-off-by: svrnm <neumanns@cisco.com>
Changes
Related to #669: By making us of the http exporter the build time for the loadgenerator can be brought down again, e.g. on a M1Pro to 23.8s, because most of the time was spend on the grpcio wheel (up to 2 minutes...)
Merge Requirements
CHANGELOG.md
updated to document new feature additions