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

Add configurable clean for .theia/logs folder. #6866

Closed
ira-gordin-sap opened this issue Jan 12, 2020 · 16 comments · Fixed by #6956
Closed

Add configurable clean for .theia/logs folder. #6866

ira-gordin-sap opened this issue Jan 12, 2020 · 16 comments · Fixed by #6956
Labels
enhancement issues that are enhancements to current functionality - nice to haves vscode issues related to VSCode compatibility

Comments

@ira-gordin-sap
Copy link

ira-gordin-sap commented Jan 12, 2020

Description

.theia/logs folder is not cleaned.

Reproduction Steps

Create VS code extension which writes logs to context.logPath received in activate() method.
In my VS Code extension I have:
export function activate(context: vscode.ExtensionContext).
The value of context.logPath on my environment is .theia/logs/20200109T092254/host/.. I use this folder to create my logs files there.

OS and Theia version:
OS: Linux
Theia version: v0.14.0

Diagnostics:
"ls .theia/logs/" shows many folders named as timestamps like 20200109T092254. Currently I see that the oldest one is from the time I created my container.

@vince-fugnitto
Copy link
Member

@ira-gordin-sap
Thanks for the issue, do you know the current behavior in VS Code?
When are log files cleaned? At which interval? I assume log files are generally cleaned when an extension is deactivated/uninstalled.

@vince-fugnitto vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves vscode issues related to VSCode compatibility labels Jan 12, 2020
@ira-gordin-sap
Copy link
Author

I don't know the current behavior in VS Code.
I am not sure it should happen when an extension is deactivated/uninstalled. I am not sure when deactivate happens on theia.
I thought about automatic clean once a configurable time.

@vince-fugnitto
Copy link
Member

@ira-gordin-sap when testing, which plugins were you using?
I'm trying to determine if it is the responsibility of the plugins to cleanup their log files, or the application itself at some point (and which point/interval).

@ira-gordin-sap
Copy link
Author

@vince-fugnitto I am implementing log in my VS code extension.
My colleague have found here: microsoft/vscode#75539 that VS Code retains logs of last 10 sessions. (I verified it on my environment.)
The relevant code is here: https://github.com/Microsoft/vscode/blob/613447d6b3f458ef7fee227e3876303bf5184580/src/vs/code/electron-browser/sharedProcess/contrib/logsDataCleaner.ts#L12

@vince-fugnitto
Copy link
Member

microsoft/vscode#75539

Thank you for investigating further and following up.
Is it something you'd like to handle? PRs are always welcome and you can copy the code from VS Code directly so long as a Contribution Questionnaire (CQ) is registered.

@bd82
Copy link
Contributor

bd82 commented Jan 14, 2020

you can copy the code from VS Code directly

Are these APIs available directly in Theia or should alternatives be used?

import { IEnvironmentService } from 'vs/platform/environment/common/environment';
import { join, dirname, basename } from 'vs/base/common/path';
import { readdir, rimraf } from 'vs/base/node/pfs';
import { onUnexpectedError } from 'vs/base/common/errors';
import { Disposable, toDisposable } from 'vs/base/common/lifecycle';

@vince-fugnitto
Copy link
Member

you can copy the code from VS Code directly

Are these APIs available directly in Theia or should alternatives be used?

import { IEnvironmentService } from 'vs/platform/environment/common/environment';
import { join, dirname, basename } from 'vs/base/common/path';
import { readdir, rimraf } from 'vs/base/node/pfs';
import { onUnexpectedError } from 'vs/base/common/errors';
import { Disposable, toDisposable } from 'vs/base/common/lifecycle';

Looking at the code previously referenced, there does not seem to be anything preventing getting the feature to work properly. There are definitely internal Theia methods and APIs that can be used to compliment the feature. Please note that if a straight copy+paste cannot be used, you can always inspire yourself from their implementation.

@ira-gordin-sap
Copy link
Author

microsoft/vscode#75539

Thank you for investigating further and following up.
Is it something you'd like to handle? PRs are always welcome and you can copy the code from VS Code directly so long as a Contribution Questionnaire (CQ) is registered.

I would very much like to handle. Waiting for approval from my product owner.

@vince-fugnitto
Copy link
Member

microsoft/vscode#75539

Thank you for investigating further and following up.
Is it something you'd like to handle? PRs are always welcome and you can copy the code from VS Code directly so long as a Contribution Questionnaire (CQ) is registered.

I would very much like to handle. Waiting for approval from my product owner.

Sounds great!

@bd82
Copy link
Contributor

bd82 commented Jan 22, 2020

Hello.

I have a fix here:

However I have reservations regarding the Eclipse Foundation Licensing.

  • https://www.eclipse.org/legal/ECA.php
  • Particularly as the filling the ECA seems to require a providing a physical postal address.
  • Combined with waving the rights related to personal information (GDPR)

I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my signoff) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Does providing a Physical Postal Address Mandatory to Sign the eclipse ECA?
Is this a question that should be forwarded to eclipse foundation legal department?

@bd82
Copy link
Contributor

bd82 commented Jan 22, 2020

Could my Employer's (who I am making the contribution on behalf of) address be provided?
Even more generally an address of my Employer in a different country than the one I reside in?

@vince-fugnitto
Copy link
Member

@marcdumais-work perhaps you can help regarding the Eclipse ECA questions?
@bd82 when I signed up, I did not provide any address information as I believe it is unnecessary (although I'm not sure if this policy has changed).

@marcdumais-work
Copy link
Contributor

Hi @bd82,

Could my Employer's (who I am making the contribution on behalf of) address be provided?
Even more generally an address of my Employer in a different country than the one I reside in?

First I do not speak for the Eclipse Foundation and IINAL. If you feel the need, we can involve someone from the foundation to get a more definitive answer.

Looking at my own ECA, I provided the address of my employer, so I think that's fine if you contribute in the context of your work. As far as I know, the country of your employer being different from your own would not matter - your employer's address is what it is.

@bd82
Copy link
Contributor

bd82 commented Jan 22, 2020

Looking at my own ECA, I provided the address of my employer, so I think that's fine if you contribute in the context of your work. As far as I know, the country of your employer being different from your own would not matter - your employer's address is what it is.

O.k. sounds good enough. I will continue creating the PR tomorrow.

Cheers.

@bd82
Copy link
Contributor

bd82 commented Jan 23, 2020

o.k. I used my employer's physical address.

A PR was created: #6956

This could be used as a stepping stone to include also configurable options: e.g:

  • Instead of hard-coded 10 last sessions, keep X last sessions.

Cheers.
Shahar.

@marcdumais-work
Copy link
Contributor

o.k. I used my employer's physical address.

The ECA check on your PR passed, so you're all set

vince-fugnitto pushed a commit that referenced this issue Jan 29, 2020
By default n===10.

N is Configurable using `--plugin-max-session-logs-folders=N`
CLI option.

fixes #6866

Signed-off-by: I060847 <shachar.soel@sap.com>
akosyakov pushed a commit to akosyakov/theia that referenced this issue Feb 24, 2020
By default n===10.

N is Configurable using `--plugin-max-session-logs-folders=N`
CLI option.

fixes eclipse-theia#6866

Signed-off-by: I060847 <shachar.soel@sap.com>
JesterOrNot pushed a commit to JesterOrNot/theia that referenced this issue Mar 12, 2020
By default n===10.

N is Configurable using `--plugin-max-session-logs-folders=N`
CLI option.

fixes eclipse-theia#6866

Signed-off-by: I060847 <shachar.soel@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants