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

removed unnecessary logger #74

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

hsubramanianaks
Copy link
Collaborator

This PR is to remove the logged telemetry events which aren't used much. And changed async line of code below for more readability.

KubectlClient_Command,
AccountContextManager_KubeconfigChange

@hsubramanianaks hsubramanianaks marked this pull request as ready for review September 25, 2023 18:17
Copy link
Contributor

@Vidya2606 Vidya2606 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #74 (d7e28e1) into main (ef2b003) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
- Coverage   66.93%   66.84%   -0.09%     
==========================================
  Files          33       33              
  Lines        2752     2745       -7     
  Branches      105      105              
==========================================
- Hits         1842     1835       -7     
  Misses        909      909              
  Partials        1        1              
Flag Coverage Δ
unittests 66.84% <0.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/clients/KubectlClient.ts 49.00% <ø> (-0.84%) ⬇️
src/logger/TelemetryEvent.ts 100.00% <ø> (ø)
src/models/context/AccountContextManager.ts 60.60% <0.00%> (ø)

}
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

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

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 ?

Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

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

🎊 Sounds good, we should make sure this is well tested and wider folks are aware of this. Thank you so much. Looks Good.

@hsubramanianaks hsubramanianaks merged commit 2c815d0 into Azure:main Oct 27, 2023
3 checks passed
@hsubramanianaks hsubramanianaks deleted the telemetry-update branch October 27, 2023 20:25
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.

5 participants