-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
test(node): Add prisma OpenTelemetry integration tests. #10778
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
size-limit report 📦
|
fc9d3a9 to
9761634
Compare
| if (NODE_VERSION.major && NODE_VERSION.major < 16) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn(`Skipping Prisma tests on Node: ${NODE_VERSION.major}`); | ||
| process.exit(0); | ||
| } | ||
|
|
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 prisma only support Node 16+? (If so, can we add a comment that this is the reason why we skip?
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.
Yes, I just added a comment with the link 👍
0ca9edd to
4c63dec
Compare
Lms24
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.
Despite my comment, I think we can merge the PR as it is right now. If the origin should be something else than 'manual' we can (and should) adjust it and the tests in a follow up PR.
| model: 'User', | ||
| name: 'User.create', | ||
| 'otel.kind': 'INTERNAL', | ||
| 'sentry.origin': 'manual', |
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.
hmm not a hard blocker to merge the tests but @mydea is it expected that the spans all have 'sentry.origin': 'manual'? Maybe this is an otel<>sentry limitation I'm not aware of.
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's because sentry.origin is the default, I think this is fine to leave for now and we re-evaluate closer to v8 release.
Replaces #10483 which I closed by mistake.
Part of #9907