-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(otel-node): instr-tedious #144
Conversation
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.
Just some nits.
// tedious@11 and later depend on @azure/identity v1 or v2. As of | ||
// @azure/core-rest-pipeline@1.15.0 (a dep of @azure/identity), support for | ||
// Node.js <16 has been broken. | ||
node: '>=16', |
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.
A thing to possibly watch out for here is this that we have in apm-agent-nodejs testing:
if (
(semver.gte(tediousVer, '17.0.0') && semver.lt(process.version, '18.0.0')) ||
// tedious@11 and later depend on @azure/identity v1 or v2. As of
// @azure/core-rest-pipeline@1.15.0 (a dep of @azure/identity), support for
// Node.js <16 has been broken.
(semver.gte(tediousVer, '11.0.0') && semver.lt(process.version, '16.0.0'))
) {
console.log(
`# SKIP tedious@${tediousVer} does not support node ${process.version}`,
);
process.exit();
}
At some point should we bump the tedious ver we test with to the latest? If so, then we'll need to guard with node >=18
.
Ah, I see open-telemetry/opentelemetry-js-contrib#1656
We should consider helping with that.
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.
A thing to possibly watch out for here is this that we have in apm-agent-nodejs testing:
I though about it but it felt I was anticipating things (YAGNI). I guess this will be useful for #43 so we are actually going to need it. I'll add it
We should consider helping with that.
Sure!
.github/workflows/test.yml
Outdated
image: mcr.microsoft.com/mssql/server | ||
env: | ||
ACCEPT_EULA: 'Y' | ||
SA_PASSWORD: 'Very(!)Secure' |
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.
nit:
- Maybe switch to a specific image version. Perhaps
mcr.microsoft.com/mssql/server:2022-latest
? - Perhaps also change to the "MSSQL_SA_PASSWORD" envvar per the "Configuration" section of https://hub.docker.com/_/microsoft-mssql-server ?
If so, would need to change in docker-compose.yaml and use-tedious.js below.
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.
fine for me :)
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Closes: #34