feat(node): Add amqplibIntegration #13714
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Could you share some examples traces with this instrumentation active?
Let's also make sure that the spans generated from this instrumentation show up in the Queues Module in Sentry.
You can read more about the requirements for the queue module here: https://docs.sentry.io/platforms/javascript/guides/node/tracing/instrumentation/custom-instrumentation/queues-module/
dev-packages/node-integration-tests/suites/tracing/amqplib/package.json
Outdated
Show resolved
Hide resolved
dev-packages/node-integration-tests/suites/tracing/amqplib/setup.ts
Outdated
Show resolved
Hide resolved
dev-packages/node-integration-tests/suites/tracing/amqplib/package.json
Outdated
Show resolved
Hide resolved
dev-packages/node-integration-tests/suites/tracing/amqplib/yarn.lock
Outdated
Show resolved
Hide resolved
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
dev-packages/node-integration-tests/suites/tracing/amqplib/init.ts
Outdated
Show resolved
Hide resolved
dev-packages/node-integration-tests/suites/tracing/amqplib/scenario-message.ts
Outdated
Show resolved
Hide resolved
dev-packages/node-integration-tests/suites/tracing/amqplib/test.ts
Outdated
Show resolved
Hide resolved
| channel: Channel; | ||
| }; | ||
|
|
||
| export async function connectToRabbitMQ(): Promise<RabbitMQData> { |
There was a problem hiding this comment.
m: I would generally inline all of these into the scenario file, unless we have multiple scenarios that need to share this. If we only have a single scenario file (which is often the case) I would try to put all the scenario code into the scenario file, this makes it easier to parse tests etc. The same goes for constants, I'd also merge this into the file :)
There was a problem hiding this comment.
we might have more, I had some problem with testing one scenario and then just give up. But even though I like to keep things separated so it is easier to see things. Would rather avoid keeping all inside one file, WDYT ?
There was a problem hiding this comment.
no strong feelings! I just generally would avoid premature optimization, we can always extract stuff out when we actually need a second file, but also I won't block this as it is now, it's OK like this too, just wanted to give general feedback :)
| @@ -483,6 +483,7 @@ export function createRunner(...paths: string[]) { | |||
| method: 'get' | 'post', | |||
| path: string, | |||
| headers: Record<string, string> = {}, | |||
| data?: any, // axios accept any as data | |||
There was a problem hiding this comment.
Do we actually need these changes now? I don't think so, unless I am missing something? :) If so, I'd just not do this change, no need to add stuff we don't need (and later nobody knows anymore if this is used somewhere etc.)
There was a problem hiding this comment.
well I needed them as first I was using post request and in this way you cannot sent the body as it was missing. The post request for axios expects body as 2nd param, but here you had headers which was wrong anyway - for post requests options like headers are passed as 3rd param, so imho this was bug anyway. I would leave it then
obecny
changed the title
AbhiPrasad
changed the title
|
@obecny I changed the title to match our conventions - we typically make the scope of the PR title be the main package that is being updated (in this case, |
thank you :) |
resolves #13312
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).