separate env vars that control number of workers

[dinmukhamedm]

Important

Separate environment variables for controlling the number of workers for spans and browser events in main.rs.

• Environment Variables:
  • Introduced NUM_SPANS_WORKERS_PER_THREAD and NUM_BROWSER_EVENTS_WORKERS_PER_THREAD in main.rs to separately control the number of workers for spans and browser events.
  • Default values set to 4 for both if environment variables are not provided.
• Worker Spawning:
  • process_queue_spans now uses NUM_SPANS_WORKERS_PER_THREAD.
  • process_browser_events now uses NUM_BROWSER_EVENTS_WORKERS_PER_THREAD.

^{This description was created by for 15e5448. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 806076f in 1 minute and 34 seconds

More details

• Looked at 37 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. app-server/src/main.rs:425

• Draft comment:
  New env var NUM_BROWSER_EVENTS_WORKERS_PER_THREAD with default 2. Ensure the default is appropriate for browser events workload.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that the default value for a new environment variable is appropriate for the workload. This is a request for confirmation or double-checking, which violates the rules.

2. app-server/src/main.rs:420

• Draft comment:
  Default value changed from '8' to '2' for NUM_WORKERS_PER_THREAD (now used for trace workers). Confirm if this change is intentional.
• Reason this comment was not posted:
  Marked as duplicate.

3. app-server/src/main.rs:425

• Draft comment:
  Consider renaming NUM_WORKERS_PER_THREAD to NUM_TRACE_WORKERS_PER_THREAD for clarity and documenting both this and NUM_BROWSER_EVENTS_WORKERS_PER_THREAD.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_7G9RwEfdIf1x81m3

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Default worker count for trace workers changed from 8 to 2. Confirm if this change is intentional and consider renaming the env var to NUM_TRACE_WORKERS_PER_THREAD for clarity.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 15e5448 in 44 seconds

More details

• Looked at 29 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. app-server/src/main.rs:420

• Draft comment:
  Renamed variable 'num_trace_workers_per_thread' to 'num_spans_workers_per_thread' and updated the env var to 'NUM_SPANS_WORKERS_PER_THREAD' with a default of 4 instead of 2. Confirm that these new defaults and naming are intentional.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to confirm their intention regarding the renaming of a variable and updating an environment variable. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.

2. app-server/src/main.rs:425

• Draft comment:
  Default for 'NUM_BROWSER_EVENTS_WORKERS_PER_THREAD' was also updated from 2 to 4. Verify that doubling the worker count is expected.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
  The comment is asking the PR author to verify if doubling the worker count is expected. This violates the rule against asking the author to confirm their intention or to ensure the behavior is intended. However, it does point out a specific change in the code, which could be useful if rephrased to suggest a review or consideration of the change's impact.

3. app-server/src/main.rs:420

• Draft comment:
  Renamed env var from NUM_WORKERS_PER_THREAD to NUM_SPANS_WORKERS_PER_THREAD with an increased default (from 2 to 4). Confirm that this intentional rename & default change aligns with your design.
• Reason this comment was not posted:
  Marked as duplicate.

4. app-server/src/main.rs:425

• Draft comment:
  Default for NUM_BROWSER_EVENTS_WORKERS_PER_THREAD was updated from 2 to 4. Ensure this is the intended behavior for browser events workload.
• Reason this comment was not posted:
  Marked as duplicate.

5. app-server/src/main.rs:431

• Draft comment:
  for-loop now iterates using num_spans_workers_per_thread, reflecting the renaming. Verify that process_queue_spans can handle the increased parallelism if the default is now 4.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to verify if a function can handle increased parallelism due to a change in the loop iteration variable. This falls under asking the author to ensure behavior is intended or to double-check things, which is against the rules.

Workflow ID: wflow_zgBwJSeHPAOwDT5x

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.