-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(instrumentation): correctly async / await Git operations
#39653
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
A previous refactor had incorrectly made it so we were not correctly `await`ing any calls to the SimpleGit method calls. This has mostly led to the Git operations continuing to work, but the instrumentation hasn't been correctly rendering. This also corrects incorrect return types that were previously not returned correctly.
viceice
left a comment
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 don't understand why those changese makes a difference, see comment. 🤷♂️
| return instrument( | ||
| spanName, | ||
| () => this.git.clone(repo, dir, options), | ||
| async () => await this.git.clone(repo, dir, options), |
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 wonder why this maks a difference. 🤔
renovate/lib/instrumentation/index.ts
Lines 185 to 196 in 92e176a
| if (ret instanceof Promise) { | |
| return ret | |
| .catch((e) => { | |
| span.recordException(e); | |
| span.setStatus({ | |
| code: SpanStatusCode.ERROR, | |
| message: massageThrowable(e), | |
| }); | |
| throw e; | |
| }) | |
| .finally(() => span.end()) as ReturnType<F>; | |
| } |
this should handle the response properly
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.
wait, maybe simple git isn't returning a proper Promise so it doesn't get into the if condition.
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.
Ah, so possibly because it returns:
export type Response<T> = SimpleGit & Promise<T>;So maybe if we refactored ☝️ to also look at instanceof Response, it'd work?
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.
It looks like the ordering if the types (when the object is constructed) can catch us out here - I've got a reproducing unit test I'm trying to get passing then will see if that fixes it
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.
When adding:
console.log({ name, typF: typeof fn, typeR: typeof ret, fn, ret, })
I see i.e.
{
name: 'syncGit',
typF: 'function',
typeR: 'object',
fn: [Function (anonymous)],
ret: Promise {
undefined,
[Symbol(async_id_symbol)]: 1857648,
[Symbol(trigger_async_id_symbol)]: 1857588,
[Symbol(kResourceStore)]: BaseContext {
_currentContext: [Map],
getValue: [Function (anonymous)],
setValue: [Function (anonymous)],
deleteValue: [Function (anonymous)]
}
}
}
{
name: 'git show',
typF: 'function',
typeR: 'object',
fn: [Function (anonymous)],
ret: Git2 {}
}
Notice that SimpleGit's calls return a Git2 (from the CommonJS code)
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.
Alternative: #39661
|
#39661 was the better fix |
|
Moved the types changes into #39664 |
Changes
A previous refactor had incorrectly made it so we were not correctly
awaiting any calls to the SimpleGit method calls.This has mostly led to the Git operations continuing to work, but the
instrumentation hasn't been correctly rendering.
This also corrects incorrect return types that were previously not
returned correctly.
Screenshots
Before (
git cloneimmediately returns):After (
git clonereturns correct timing)Context
Please select one of the below:
AI assistance disclosure
Did you use AI tools to create any part of this pull request?
Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
The public repository: