-
Notifications
You must be signed in to change notification settings - Fork 39
test: corrected and clarified bull test cases #2002
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
| // @ts-ignore - Type 'string' is not assignable to type 'undefined' | ||
| // Probably because exports.agentUuid is set to undefined and export values were not supposed to be changed | ||
| // TODO: This has no effect. Investigate further. | ||
| agentOpts.agentUuid = parentProcessAgentUuid; |
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.
If you comment out this line, the tests still work.
The corrected test logic proofs that this is not used.
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.
Another PR will come to remove it and to improve the logic in our codebase for this case.
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 was refactoring the codebase for util, config and logging and these agentOpts file was in my way and then I digged into this, because its very dirty code.
| if (uniqueAgentUuids) { | ||
| uuid = uuids[pid] = `agent-stub-uuid-${pid}`; | ||
| } else { | ||
| uuid = 'agent-stub-uuid'; |
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.
The bull tests were false positives.
The agent stub always returned "agent-stub-uuid" for ANY registered process.
The test asserted that the spans from the bull child process (bull forks a process for jobs), have this uuid for span.f.h (agentOpts.agentUuid). And this agentUuid is set to its parent process uuid (see immediate) -> but any process uses the hardcoded UUID from the agent stub "agent-stub-uuid"! So you don't know if its the actual parent uuid or the child uuid.
Each process gets now its own uuid.
I have updated the tests to proof that the INSTANA_AGENT_UUID has no effect and is not used.
| if (f.startsWith('file-created-by-job') && f.endsWith('.json')) { | ||
| fs.unlinkSync(path.join(__dirname, f)); | ||
| } | ||
| }); |
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.
Cleanup json files before starting. This was missing.
|
|
||
| globalAgent.setUpCleanUpHooks(); | ||
| const agentControls = globalAgent.instance; | ||
| await customAgentControls.startAgent({ |
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.
Custom agent with using the new uniqueAgentUuids option.
| // 2 x http exit | ||
| // 2 x redis | ||
| // 4 x otel (read + write for the job files) | ||
| spanLength: 13, |
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.
There is more otel spans now because we read + write more to the the file system. See util.js.
| fileCreatedByJob = path.join(__dirname, 'file-created-by-job.json'); | ||
| } | ||
|
|
||
| jobData.pid = process.pid; |
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.
Assign pid to job data file to get access to the forked bull child pids.
| return originalProcessJob.apply(ctx, originalArgs); | ||
| } | ||
|
|
||
| // TODO: The entry is CREATED BEFORE the child process is forked. Its created ON THE receiver process. |
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.
This is super weird. I won't look into it, but its good to know this. I guess the decision was made because there was no way to create the entry on the child process - but not sure.
|
|
||
| if (jobData.data.bulkIndex) { | ||
| fileCreatedByJob = path.join(__dirname, `file-created-by-job-${jobData.data.bulkIndex}.json`); | ||
| } else if (jobData.opts.jobId?.includes('repeat')) { |
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 logic to store the job files for repeat case. Not really interesting
| log(`Job named ${jobName} and concurrent`); | ||
| currentTypeArgs.unshift(jobName, NUMBER_OF_PROCESSES); | ||
| } else if (jobName && !isConcurrent) { | ||
| // You cannot set name + concurrency at the same time. The concurrency is ignored in that case. |
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.
All tests used BULL_CONCURRENCY_ENABLED=true but it had no effect because of ^ 😭
| log(`Job named ${jobName}, not concurrent`); | ||
| currentTypeArgs.unshift(jobName); | ||
| } else if (!jobName && isConcurrent) { | ||
| // TODO: We don't have a test for this yet. |
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.
No test cases for concurrency - that means one job two processes.
| BULL_RECEIVE_TYPE: receiveMethod, | ||
| BULL_JOB_NAME: 'steve', | ||
| BULL_JOB_NAME_ENABLED: 'true', | ||
| BULL_CONCURRENCY_ENABLED: 'true' |
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.
aryamohanan
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.
pre-approving..
|
The tests are not exited properly. Please ensure before merging. |
| response, | ||
| apiPath, | ||
| spanLength: 17, | ||
| spanLength: 19, |
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.
Why did this change?
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.
👍 Custom Agent was not stopped |
5bb30f5 to
1dd0ded
Compare
See comments.