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 websockets for sending logs if available #489

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

rc10house
Copy link
Contributor

Adds functionality to main.ts to check if the provided config.url is running a WebSocket server. If so, we upgrade the connection to send data over WebSockets and use this for sending all logs. If not, we send data with the existing REST functionality. I also added a test/example WebSockets server examples/ws-server.js.

Note: WebSockets will not run when using Jest to test currently, so the application skips attempting to create a WebSockets connection if it detects running in a test environment (headless browser).

Closes: #444

src/main.ts Show resolved Hide resolved
src/main.ts Show resolved Hide resolved
example/test-client.ts Outdated Show resolved Hide resolved
@rc10house rc10house requested a review from Jyyjy August 21, 2024 20:23
@rc10house
Copy link
Contributor Author

Had to separate some tests to avoid websockets being ran from within jsdom environment. Also had to add some helper methods for manually turning on/off websockets for testing purposes.

@Jyyjy
Copy link
Contributor

Jyyjy commented Aug 29, 2024

You can add another webserver to test/e2e/playwright.config.ts and specify which webserver to use for different projects. Set existing projects to use the existing webserver, and create a new project with a websocket server.

I would set http and websockets to different ports within the same server and specify "reuseExistingServer: True"

Relevant docs:
https://playwright.dev/docs/test-webserver

@EandrewJones
Copy link
Contributor

@rc10house Bumping this. Would be nice to get this over the finish line if you have time.

Copy link
Contributor

@Jyyjy Jyyjy left a comment

Choose a reason for hiding this comment

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

Good test coverage, thanks for being persistent. For my own understanding, what's the difference between jest specs that have "@jest-environment jsdom" and those that don't?

@rc10house
Copy link
Contributor Author

@Jyyjy for some reason, the previous jest-environment specification (in package.json) lead to some of the tests using node as the environment anyway. Node would then throw errors because it tries to use ws module, which is not available in the browser. I added the @jest-environment jsdom individually to avoid these errors.

@rc10house
Copy link
Contributor Author

@Jyyjy the specific tests with the added @jest-environment jsdom were the ones throwing the node errors.

@Jyyjy
Copy link
Contributor

Jyyjy commented Oct 16, 2024

@EandrewJones gonna leave this up for a couple days in case you want to review, if not I'll merge.

@EandrewJones
Copy link
Contributor

EandrewJones commented Oct 16, 2024 via email

@Jyyjy Jyyjy merged commit ea17796 into apache:master Oct 16, 2024
4 checks passed
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.

Feature(streaming): support streaming logs with websocket
3 participants