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

Added web browser traffic in loadgenerator #1266

Merged
merged 9 commits into from
Nov 28, 2023

Conversation

jordibisbal8
Copy link
Contributor

@jordibisbal8 jordibisbal8 commented Nov 17, 2023

Changes

In this PR, we aim to add browser traffic by using playwright (as discussed here). The main changes are:

  • Installed playwright in the DockerFile
  • Increase memory constraints for the load generator container to be able to run playwright for several concurrent users
  • Added playwright user and two tasks in the locustfile. One goes to the /cart page and the other clicks the Go shopping button.
  • Added environment variable to enable browser traffic (false by default)

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

Copy link
Contributor

@danielgblanco danielgblanco left a comment

Choose a reason for hiding this comment

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

I've left a couple of comments (not a maintainer to approve). I think this is a great addition and will help build up client-side instrumentation use cases.

Copy link

linux-foundation-easycla bot commented Nov 17, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@jordibisbal8 jordibisbal8 marked this pull request as ready for review November 17, 2023 15:34
@jordibisbal8 jordibisbal8 requested a review from a team November 17, 2023 15:34
@julianocosta89
Copy link
Member

This is does not build.
greenlet needs to be downgraded to v 3.0.0

@jordibisbal8
Copy link
Contributor Author

This is does not build. greenlet needs to be downgraded to v 3.0.0

Thanks for spotting this @julianocosta89, I have downgraded greenlet

@jordibisbal8
Copy link
Contributor Author

@julianocosta89 can you have another look now?

@austinlparker austinlparker merged commit fd448b5 into open-telemetry:main Nov 28, 2023
27 checks passed
@julianocosta89
Copy link
Member

Hello @jordibisbal8 and @danielgblanco 👋🏽

I've been playing around with the demo since this PR got merged and I couldn't figure it out what that added to the demo.
I was expecting that after enabling LOCUST_BROWSER_TRAFFIC_ENABLED I would get traces where the root span would be the frontend-web.

It happens that this is not happening, and I only get the frontend-web spans, when manually navigating through the browser.

If that's not what this PR added, could you help me out understanding here what Playwright actually brings to the demo?

This was a big change on memory allocation, I just want to make sure we are getting something out of it.

FYI: @austinlparker

@jordibisbal8
Copy link
Contributor Author

jordibisbal8 commented Dec 11, 2023

Hello @jordibisbal8 and @danielgblanco 👋🏽

I've been playing around with the demo since this PR got merged and I couldn't figure it out what that added to the demo. I was expecting that after enabling LOCUST_BROWSER_TRAFFIC_ENABLED I would get traces where the root span would be the frontend-web.

It happens that this is not happening, and I only get the frontend-web spans, when manually navigating through the browser.

If that's not what this PR added, could you help me out understanding here what Playwright actually brings to the demo?

This was a big change on memory allocation, I just want to make sure we are getting something out of it.

FYI: @austinlparker

Yes, you should be able to see traces with the loadgenerator calling the browser. Let me test this again and I will let you know :) How are you setting the environment up? docker-compose?

@julianocosta89
Copy link
Member

Yes, you should be able to see traces with the loadgenerator calling the browser. Let me test this again and I will let you know :) How are you setting the environment up? docker-compose?

Yes:

docker compose build
docker compose up

@jordibisbal8
Copy link
Contributor Author

Screenshot 2023-12-12 at 10 09 26 Do you see the browser events/tasks in the load generator? Like the screenshot I have attached :)

@puckpuck
Copy link
Contributor

I have also noticed a similar experience as @julianocosta89 and did a lot more digging to figure it out.

The current scripts for the playwright user scenarios, hitting the cart endpoint or going home then clicking a button, don't generate a second request. The scenario of going home and hitting a button is navigating to a link anchor built into the home page and not doing another request.

I modified the script to click on a product and then the add to cart button so that it could generate multiple requests. This worked and did produce traces, but they were all missing root spans. This leads me to believe the playwright Javascript engine doesn't properly work with the OpenTelemetry SDK, and that's why we do not see any traces that have a service of frontend-web.

It makes sense to open a new issue for this and track it. I don't like the idea of having a 10x memory requirement for the load generator if we don't produce traces that originate from frontend-web. I would like to get this resolved before we release.

@jordibisbal8
Copy link
Contributor Author

jordibisbal8 commented Dec 13, 2023

@puckpuck thanks for reporting this issue, I will take a closer look :)

@danielgblanco
Copy link
Contributor

I believe the issue is related to having a hardcoded PUBLIC_OTEL_EXPORTER_OTLP_TRACES_ENDPOINT pointing to localhost in

PUBLIC_OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=http://localhost:8080/otlp-http/v1/traces

and also in the chart values

https://github.com/open-telemetry/opentelemetry-helm-charts/blob/764920aac4d1fe1147ba187dd86631e021c2e297/charts/opentelemetry-demo/values.yaml#L343

In both cases, it expects the collector to be available in localhost. In the K8s example, expecting port-forward to be run beforehand to achieve this. I can see two main options:

  1. Making the same host routable from within the loadgenerator container and from outside a cluster (hitting the browser manually). This may add extra complexity.
  2. Using a different PUBLIC_OTEL_EXPORTER_OTLP_TRACES_ENDPOINT depending on the type of request. As there is a
    ctx = baggage.set_baggage("synthetic_request", "true")
    this could be doable and we have a different endpoint for those after checking baggage.

@danielgblanco
Copy link
Contributor

The issue @puckpuck related to missing parents could be a timing issue though, possibly related to open-telemetry/opentelemetry-js#2205. Would be good to execute some requests, then navigate away to another page (so unload is triggered) and wait for load or for network idle before finishing the test case. This should ensure that spans are exported.

So, another PR on this would need:

  1. For the frontend to know when a request is coming from loadgen (via synthetic_request baggage) and use a different PUBLIC_OTEL_EXPORTER_OTLP_TRACES_ENDPOINT (should be configurable via a new env property).
  2. Some tests that generate some XHR requests that can be instrumented (not just the initial page load).
  3. Trigger an unload and wait for the browser to push spans (i.e. wait for next load or network idle).

@danielgblanco
Copy link
Contributor

Actually, when the batch processor is configured here

https://github.com/open-telemetry/opentelemetry-demo/blob/889d455951ac091d9926d151f540d33e4d844b63/src/frontend/utils/telemetry/FrontendTracer.ts#L34C13-L34C13

We could set a different exportTimeoutMillis (or via env var) that allows us to control how long to wait on the Locust side for spans to be exported.

jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* Added playwright on load generator docker image

* Added locust plugins as a dependency

* Increased memory constrains and introduced LOCUST_BROWSER_TRAFFIC_ENABLED environment variable

* Added playwright user to generate browser traffic

* Updated changelog message

* Added skipping line + renamed event

* Downgraded greenlet to 3.0.0

---------

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
Co-authored-by: Austin Parker <austin@ap2.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants