-
Notifications
You must be signed in to change notification settings - Fork 749
telemetry(auth): move debugging telemetry upstream for aws_loadCredentials
#6541
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
Conversation
d87e00b to
b71d84a
Compare
a30f03b to
f3bf546
Compare
| } | ||
|
|
||
| void this.emitWithDebounce({ | ||
| telemetry.aws_loadCredentials.emit({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does removing the debounce here mean we think this piece is no longer the issue? the debounce is moved higher up the stack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, and now behaves slightly differently.
|
TBH i'm not too concerned about having to put debugging telemetry other places, I'm more scared of events like: considering how many times aws_loadCredentials was called |
|
Yeah, I agree. My thinking is that first priority is to stop spamming telemetry right now. Then, add utility for debugging the root issue. I don't think its worth the tradeoff to put more burden on telemetry to potentially debug this faster. |
| * | ||
| * Multiple calls made during the throttle window will return the last returned result. | ||
| */ | ||
| export function throttle<Input extends any[], Output>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also want to indicate that function calls will be throttled regardless of difference in input arguments
| clock.uninstall() | ||
| }) | ||
|
|
||
| it('prevents a function from executing more than once in the `delay` window', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a test that asserts the first function call is executed and resolved before the first interval even completes. I.e if we tick 1ms while the delay is 3ms, we will get a result
|
This is a lot of change, in a somewhat high-risk area,
That's the main change, right? And the other changes in this PR could be dropped. |
|
There is not enough evidence to suggest that |
… remove debugging. (#6548) ## Problem Alternative solution to #6541 ## Solution - debounce very aggressively (once per day). - remove debugging `function_call`. ## Notes debounce ignores args, so regardless of the telemetry metadata, it will only be emitted once per day. That is, regardless of the value of `credentialsSourceId`, it is emitted once per day. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
… remove debugging. (aws#6548) ## Problem Alternative solution to aws#6541 ## Solution - debounce very aggressively (once per day). - remove debugging `function_call`. ## Notes debounce ignores args, so regardless of the telemetry metadata, it will only be emitted once per day. That is, regardless of the value of `credentialsSourceId`, it is emitted once per day. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Problem
Follow up to: #6499
debounceefforts were not aggressive enough.auth.listConnectionsbeing spammed, but unfortunately this function is called in many places, making it difficult to track. Therefore, more investigative work is required.Solution
debounceup togetCredentialProviderNamesand convert it tothrottle(described below). Note that we can't move it up toauth.listConnectionsbecause it is not a "pure" function and has expected side effects. Moving this up allows us to throttle bothemitcalls to this metric more easily.function_calldebugging telemetry inside the throttle to make it less noisy.withTelemetryHelperdecorators and manual wrapping.aws_loadCredentialsis still being emitted a lot.Throttle Notes
throttleis similar todebounce, but it returns the last cached result immediately on call. See the tests and docstring for more information.Alternative Solution
function_callmetrics. Which is now much noisier thanaws_loadCredentials(but is temporary).Future Work
function_callmetric. Non-class methods can't use decorator, but perhaps we can write something else.feature/xbranches will not be squash-merged at release time.