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.

Sign up for GitHub

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

Jump to bottom

removed unnecessary logger #74

Merged
merged 2 commits into from
Oct 27, 2023
Merged

removed unnecessary logger #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ee5118
removed unnecessary logger
hsubramanianaks Sep 25, 2023
d7e28e1
remove telemetry events
hsubramanianaks Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/clients/KubectlClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,6 @@ export class KubectlClient implements IClient {
}

private async runKubectlCommandAsync(args: string[], kubeconfigPath: string, quiet: boolean = false): Promise<string> {
// Run the kubectl command.
this._logger.trace(TelemetryEvent.KubectlClient_Command, {
args: args.join(` `)
});

// We do not add the kubeconfigPath to args, as it is PII and we don't want it to be logged.
let argsWithKubeconfig: string[] = [ ...args ];
if (kubeconfigPath != null) {
Expand Down
2 changes: 0 additions & 2 deletions src/logger/TelemetryEvent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

export enum TelemetryEvent {
AccountContextManager_GetKubeconfigPathError = "account-context-manager-get-kubeconfig-path-error",
AccountContextManager_KubeconfigChange = "account-context-manager-kubeconfig-change",
Activation = "activation",
BinariesUtility_CheckIfBinaryExistsError = "binaries-utility-check-if-binary-exists-error",
BinariesUtility_CleanUpBeforeDownloadError = "binaries-utility-cleanup-before-download-error",
Expand Down Expand Up @@ -98,7 +97,6 @@ export enum TelemetryEvent {
KubeConfigCredentialsManager_RefreshCredentialsPerf = "kubeconfig-credentials-manager-refresh-credentials-perf",
KubeConfigCredentialsManager_RefreshCredentialsSuccess = "kubeconfig-credentials-manager-refresh-credentials-success",
KubectlClient_AllFqdnsRetrievalError = "kubectl-client-all-fqdns-retrieval-error",
KubectlClient_Command = "kubectl-client-command",
KubectlClient_CommandError = "kubectl-client-command-error",
KubectlClient_CommandSuccess = "kubectl-client-command-success",
KubectlClient_CurrentContextRetrievalError = "kubectl-client-current-context-retrieval-error",
Expand Down
8 changes: 4 additions & 4 deletions src/models/context/AccountContextManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,16 @@ export class AccountContextManager {
this._currentKubeconfigPath = kubeConfigPathResult.hostPath;
if (this._fsWatcher == null) {
this._fsWatcher = FileSystemWatcher.watch(this._currentKubeconfigPath);
this._fsWatcher.on(`change`, path => {
this._logger.trace(TelemetryEvent.AccountContextManager_KubeconfigChange);
this._kubeconfigChanged.trigger();
});
}
else {
this._fsWatcher.unwatch(oldKubeconfigPath);
this._fsWatcher.add(this._currentKubeconfigPath);
}
}
// The kubeconfig file is watched for changes, and the event is triggered when the file is changed.
this._fsWatcher.on(`change`, (path) => {
Copy link
Member

@Tatsinnit Tatsinnit Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

💡 this is interesting hence asking, moving this out means it will skip the if clause and what was the purpose of that if clause?

Previously it was this:

So thinking outlaud as to what was the reason and extent the change was trigger impact :) no clue, in case you know what is the reason behind this. aka why previously it was cushioned with if clause previously?

         if (this._fsWatcher == null) {
                    this._fsWatcher = FileSystemWatcher.watch(this._currentKubeconfigPath);
                    this._fsWatcher.on(`change`, path => {
                        this._logger.trace(TelemetryEvent.AccountContextManager_KubeconfigChange);
                        this._kubeconfigChanged.trigger();
                    });
                }
                else {
                    this._fsWatcher.unwatch(oldKubeconfigPath);
                    this._fsWatcher.add(this._currentKubeconfigPath);
                }

Copy link
Collaborator Author

@hsubramanianaks hsubramanianaks Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good question @Tatsinnit I was also surprised how this was logging 20M times being inside if condition. According to the library this code will execute whenever there is change event triggered on the directory or file path it's watching so if condition doesn't make sense and caused confusion here. so moved the code below for more readability,

this._fsWatcher.on(`change` - this will be triggered if there are changes in the kube config file ex: when you switch namespaces or switch clusters this will happen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Tatsinnit are we good with this ? can we resolve this and merge the PR ?

this._kubeconfigChanged.trigger()
});

return this._currentKubeconfigPath;
}
Expand Down
Loading