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

electron: allow accessing the metrics endpoint for performance analysis #13380

Merged

Conversation

xai
Copy link
Contributor

@xai xai commented Feb 12, 2024

What it does

By default, when running Theia in Electron, all endpoints are protected by the ElectronTokenValidator.
This patch allows accessing the '/metrics' endpoint without a token, which enables us to collect metrics for performance analysis.

For this, ElectronTokenValidator is extended to allow access to the metrics endpoint. All other endpoints are still protected.

Contributed on behalf of STMicroelectronics

How to test

  • Build and start the Electron example. To avoid getting a random free port on runtime, use yarn electron start --port=3000.
  • Access the '/metrics' endpoint, e.g., by calling curl http://localhost:3000/metrics.
  • Verify that metrics are returned.
  • Access a different endpoint, e.g. http://localhost:3000/foo.
  • Verify that the request is denied.

Follow-ups

Review checklist

Reminder for reviewers

@xai xai force-pushed the electron-expose-metrics-endpoint branch from f32aa41 to 4d6a313 Compare February 13, 2024 00:00
@xai xai marked this pull request as ready for review February 20, 2024 10:27
@xai xai force-pushed the electron-expose-metrics-endpoint branch from 4d6a313 to a2fa6ad Compare February 20, 2024 10:46
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

This works, however we should think about whether we really want to expose the metrics endpoint by default in Electron. I think it would be more reasonable to only do the rebinding in case we are in debug or development mode. We already only collect startup metrics in development mode, so we could align with that.

Rebinding the ElectronTokenValidator will also often not have an effect in downstream projects. It's a common pattern to rebind it to allow for custom exposed APIs, so they will overwrite this one again. We should probably have a contribution point within the ElectronTokenValidator to allow contributing whitelists. Of course this is out of scope for this PR, but we should create a ticket for that.

@tortmayr
Copy link
Contributor

tortmayr commented Feb 20, 2024

This works, however we should think about whether we really want to expose the metrics endpoint by default in Electron. I think it would be more reasonable to only do the rebinding in case we are in debug or development mode. We already only collect startup metrics in development mode, so we could align with that.

Sounds reasonable to me. But I don`t have a strong opinion on that. I don't see any harm in exposing the metrics endpoint per default.

Rebinding the ElectronTokenValidator will also often not have an effect in downstream projects. It's a common pattern to rebind it to allow for custom exposed APIs, so they will overwrite this one again. We should probably have a contribution point within the ElectronTokenValidator to allow contributing whitelists. Of course this is out of scope for this PR, but we should create a ticket for that.

+1 I agree, rebinding the ElectronTokenValidator is not ideal. I like the idea of a contribution point for whitelisting custom API endpoints.

@xai
Copy link
Contributor Author

xai commented Feb 20, 2024

Thanks, @sdirix and @tortmayr! I will revise my PR accordingly.

Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Looks good to me and works as expected.
One minor regarding imports but otherwise everything works.

return this.allowRequest(request);
}

override allowRequest(request: IncomingMessage): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should either be http.IncomingMessage
or the type in allowWsUpgrade should be aligned to avoid the double import from "http".
(import * as http from 'http' and import { IncomingMessage } from 'http'; )

By default, when running Theia in Electron, all endpoints are protected
by the ElectronTokenValidator.
This patch allows accessing the '/metrics' endpoint without a token,
which enables us to collect metrics for performance analysis.

For this, ElectronTokenValidator is extended to allow access to the
metrics endpoint. All other endpoints are still protected.

Contributed on behalf of STMicroelectronics

Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
@xai xai force-pushed the electron-expose-metrics-endpoint branch from a2fa6ad to 5017e39 Compare March 12, 2024 13:08
@JonasHelming
Copy link
Contributor

@tortmayr Can you merge this?

@tortmayr tortmayr merged commit 972d0f7 into eclipse-theia:master Mar 13, 2024
14 checks passed
@tortmayr tortmayr deleted the electron-expose-metrics-endpoint branch March 13, 2024 09:55
jonah-iden pushed a commit that referenced this pull request Mar 15, 2024
…is (#13380)

By default, when running Theia in Electron, all endpoints are protected
by the ElectronTokenValidator.
This patch allows accessing the '/metrics' endpoint without a token,
which enables us to collect metrics for performance analysis.

For this, ElectronTokenValidator is extended to allow access to the
metrics endpoint. All other endpoints are still protected.

Contributed on behalf of STMicroelectronics

Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
jonah-iden added a commit that referenced this pull request Mar 22, 2024
* basics for dev-container support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic creating and connecting to container working

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* open workspace when opening container

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* save and reuse last USed container per workspace

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* restart container if running

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* better container creation extension features

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* added dockerfile support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* rebuild container if devcontainer.json has been changed since last use

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fix build

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed checking if container needs rebuild

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* working port forwarding via exec instance

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* review changes

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fix import

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* smaller fixes and added support for multiple devcontainer configuration files

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic output window for devcontainer build

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* smaller review changes and nicer dockerfile.json detection code

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed build and docuemented implemented devcontainer.json properties

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* Fix unneeded URI conversion (#13415)

* Fix quickpick problems found in IDE testing (#13451)

Fixes #13450, #13449

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

* Fix rending of quickpick buttons (#13342)

Ensure that  the Theia specific wrapper for the MonacoQuickPickItem properly forwards assignments of the "buttons" property to the wrapped item.

Fixes #13076

Contributed on behalf of STMicroelectronics

* electron: allow accessing the metrics endpoint for performance analysis (#13380)

By default, when running Theia in Electron, all endpoints are protected
by the ElectronTokenValidator.
This patch allows accessing the '/metrics' endpoint without a token,
which enables us to collect metrics for performance analysis.

For this, ElectronTokenValidator is extended to allow access to the
metrics endpoint. All other endpoints are still protected.

Contributed on behalf of STMicroelectronics

Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>

* fixed renaming and moving of open notebooks (#13467)

* fixed renameing of open notebooks

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed moving of notebook editors to other areas

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

---------

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* [playwright] Update documentation

Since a recent enhancement/refactoring of @theia/playwright, to permit using it in Theia
Electron applications, the way to load an application has changed. This commit is an
attempt to update the examples that are part of the documentation. I validated the changes
in the "theia-playwright-template" repository, and so I have adapted the sample code to
that repo's linting rules (using single quotes instead of double).

It's possible that other things have changed, that I have not yet encountered, but this
should be a good step forward, at least for those just getting started integrating
playwright to test their Theia-based app.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>

* basics for dev-container support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic creating and connecting to container working

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* added dockerfile support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* added port forwarding inlcuding ui

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic port/address validation

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed allready forwarded port checking

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* rebase  fixes

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* removed unused file

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* review changes

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed widget focus and message margin

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* default port binding now shows as 0.0.0.0

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

---------

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
Co-authored-by: Alexander Taran <Alexander-Taran@users.noreply.github.com>
Co-authored-by: Thomas Mäder <tsmaeder@users.noreply.github.com>
Co-authored-by: Tobias Ortmayr <tortmayr@eclipsesource.com>
Co-authored-by: Olaf Lessenich <olessenich@eclipsesource.com>
Co-authored-by: Marc Dumais <marc.dumais@ericsson.com>
@jfaltermeier jfaltermeier added this to the 1.48.0 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants