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.

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

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Jan 15, 2023

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
  • Appropriate documentation updates in the docs folder

Signed-off-by: svrnm <neumanns@cisco.com>
@svrnm svrnm requested a review from a team as a code owner January 15, 2023 20:53
Signed-off-by: svrnm <neumanns@cisco.com>
@cartersocha
Copy link
Contributor

Another big perf increase for me. 13 vs 174 seconds on m2 mac air.

Please keep these coming lol

@cartersocha
Copy link
Contributor

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

@cartersocha
Copy link
Contributor

on a mac m2

Copy link
Member

@julianocosta89 julianocosta89 left a 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>
@svrnm
Copy link
Member Author

svrnm commented Jan 16, 2023

LGTM, there is just a trailing space in the changelog.

TY, fixed.

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

odd, what errors exactly are you getting? I can try to reproduce it on a M1

Copy link
Member

@mviitane mviitane left a 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.

@cartersocha
Copy link
Contributor

system prune did the trick. lgtm. I'm seeing feature flag service errors like mentioned in a recent issue but thats unrelated

@cartersocha cartersocha merged commit 3d8c142 into open-telemetry:main Jan 17, 2023
@puckpuck puckpuck added the helm-update-required Requires an update to the Helm chart when released label Jan 17, 2023
@imomaliev
Copy link

imomaliev commented Feb 4, 2023

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

@cartersocha
Copy link
Contributor

@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.

@imomaliev
Copy link

imomaliev commented Feb 5, 2023

@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?

@puckpuck
Copy link
Contributor

puckpuck commented Feb 5, 2023

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.

jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* 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>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants