Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Trace-extension configuration settings #184

Closed
ankusht-work opened this issue Nov 18, 2020 · 8 comments · Fixed by #198
Closed

Add Trace-extension configuration settings #184

ankusht-work opened this issue Nov 18, 2020 · 8 comments · Fixed by #198
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@ankusht-work
Copy link
Contributor

ToDo:

  1. Add new preferences settings that will take two parameters a) path to the executable b) port on which user would like to spawn the server.
  2. Implement an event-listener that will monitor the changes in trace-server preferences.
  3. Spawn the server at the start of the application and/or whenever the preferences settings are modified?---Need more investigation on this.

The draft of the preferences UI can be seen in the image attached. Any feedback is much appreciated.
Config

@ebugden
Copy link
Contributor

ebugden commented Dec 2, 2020

@bhufmann @mirsky-work @tahini Front-end users having control over starting and stopping the trace server seems to overstep the front end's responsibilities and also contradict how the tool is expected to be used. Here are a couple examples where users having control of the trace server doesn't make sense:

  • A trace server providing traces to multiple clients simultaneously Support multi-user in the trace server #21 (any client could brake the other's sessions)
  • A single user connecting to and reading traces from several different remote servers (these servers don't "belong" to the user)

From my perspective, it makes sense that users would be able to configure which server(s) they want to connect to (like you can configure the list of servers the Ubuntu package manager pulls from), but not that any user could start/stop servers from the front end.

I vote for keeping configuring/managing the server in the command line for now. Semantically, trace server management is a very different set of responsibilities.

Also related: Issue #53 Use the Theia backend to automatically start the trace server

@bhufmann
Copy link
Collaborator

bhufmann commented Dec 2, 2020

@ebugden I think we need to distinguish the 2 cases

  1. a trace server is deployed as a service and shared by multiple users, i.e. cloud deployment
  2. a dedicated trace server per user (and per Theia instance) that runs where the workspace is.

The cloud deployment (case 1) is a long term goal and I agree that starting, stopping and configuration of the trace server is not the responsibility of the user and the user can't have control over the server.

Case 2 is similar how language servers and debug servers work. There is one server per user (and per Theia instance), which is obviously not shared between users. This is the case that is currently under development. The trace server should be automatically be started when the user opens a trace and stopped when it's not needed anymore (like language servers).

As far as I understood, giving the user the option to start and stop its trace server is to due to some issues with automatically starting/stopping of the server and that solution is for the short-term. @ankusht-work @mirsky-work could you please provide some details about the issues?

@ankusht-work
Copy link
Contributor Author

@ebugden : As Bernd mentioned, this work is intended for a single-user/single-server scenario. We will have to re-visit this when we add the multi-user use case. IMO, for now, this approach is slightly better than having to run the server via CLI.

@mirsky-work
Copy link
Contributor

The motivation behind this feature is to simplify the work-flow for the users, focusing on the local use-case and server-per-user. The Path in the screenshot above is for a filesystem local to the workspace. In the future we'll need to add a URL field to allow connecting to an already running server (remote, or local). IMO, explicitly starting the server will be useful in the future to distinguish between the two use-cases, and I believe it's a viable incremental improvement, even if we consider doing so automatically.

@ebugden ebugden added this to the First MVP milestone Dec 3, 2020
@ebugden
Copy link
Contributor

ebugden commented Dec 3, 2020

Sounds good, I'm convinced! I agree that this makes sense as a quality of life improvement for the one server per user use case.

In the future I think it's possible that users:

  • Could get annoyed if configuration settings change
  • Could be comparing local and remote traces simultaneously

But we can cross those bridges when we get there.

@ebugden
Copy link
Contributor

ebugden commented Dec 3, 2020

Could the configuration settings section be labeled "Trace Viewer Server"? It's more specific.

@mirsky-work
Copy link
Contributor

Could the configuration settings section be labeled "Trace Viewer Server"? It's more specific.

But settings are of the Trace Viewer plugin. One of the settings is the server details, but there might be others, related purely to the front-end, e.g., timestamp formatting.

ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 3, 2020
Set the default values of the trace server preferences
Read the values and spwan the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi ankush.tyagi@ericsson.com
@ebugden
Copy link
Contributor

ebugden commented Dec 4, 2020

Settings are of the Trace Viewer plugin. One of the settings is the server details, but there might be others, related purely to the front-end, e.g., timestamp formatting.

I agree that future additional settings can be unrelated to the trace server, but unless adding these are planned for the near future, specifying that these are "Trace Viewer Server" settings adds clarity and reassurance for users in the meantime.

If more general settings are added later, it's quick to change the name back to "Trace Viewer" in the same pull request that adds those new settings.

ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 7, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi ankush.tyagi@ericsson.com
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 7, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi ankush.tyagi@ericsson.com
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 7, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 7, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 7, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 15, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 17, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 28, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
ankusht-work added a commit to ankusht-work/theia-trace-extension that referenced this issue Dec 28, 2020
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
bhufmann pushed a commit that referenced this issue Jan 12, 2021
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes #191
fixes #184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
hriday-panchasara pushed a commit to hriday-panchasara/theia-trace-extension that referenced this issue Nov 10, 2021
Set the default values for the trace server preferences
Read the trace server preference values and spawn the server from the path specified in the
preferences

fixes eclipse-cdt-cloud#191
fixes eclipse-cdt-cloud#184

Signed-off-by: Ankush Tyagi <ankush.tyagi@ericsson.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants