Use our forked analytics-python

[shadinaif]

Change description

Use our forked analytics-python

Type of change

• Bug fix (fixes an issue)
• New feature (adds functionality)

Related issues

https://appsembler.atlassian.net/browse/RED-2969

Checklists

Development

• Lint rules pass locally
• Application changes have been tested thoroughly
• Automated tests covering modified code pass

Security

• Security impact of change has been considered
• Code follows company security practices and guidelines

Code review

• Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
• "Ready for review" label attached and reviewers assigned
• Changes have been reviewed by at least one other contributor
• Pull request linked to task tracker where applicable

[github-actions]

Checking git merge conflicts against https://github.com/edx/edx-platform.git

Comparing with	open-release/koa.master
Benchmark conflicts with main	101
Current conflicts	101
Summary	Good work! No added conflicts.

Comparing with	master
Benchmark conflicts with main	293
Current conflicts	293
Summary	Good work! No added conflicts.

[coveralls]

Pull Request Test Coverage Report for Build 2515013695

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.002%) to 48.667%

---

Files with Coverage Reduction	New Missed Lines	%
common/lib/xmodule/xmodule/modulestore/xml.py	3	77.94%

Totals
Change from base Build 2504379865:	0.002%
Covered Lines:	109247
Relevant Lines:	224478

---

💛 - Coveralls

[OmarIthawi]

~

[OmarIthawi]

first of all, thanks for getting to the bottom of this bug.

this is a big change, could you please open a upstream pull request and explain why do you think this is a bug? I know that we've already forked the repo but the change isn't trivial to test or review.

This change seems very dangerous since it touches threading/multiprocessing
think about: servers going out of memory, site data leaks or bringing servers down.
such changes actually shouldn't be done without consulting with the package maintainers in my opinion.
i.e. I'd rather accept the bug and wait for a couple of months to ensure we did the right thing instead of potentially hurting our production instances.

[OmarIthawi]

in other words @shadinaif. I don't understand well the changes done appsembler/analytics-python#2 . it seems too good to be true it raises a red-flag because we don't really know the implication of the change.

[OmarIthawi]

I'm not angry, I'm just trying to scrap my head about the change. Please check the links below becasue I think we have a sync_mode which skips queueing:

• Add sync_mode option segmentio/analytics-python#147
• Synchronous Mode segmentio/analytics-python#101
• search results for "celery" in segmentio/analytics-python

[shadinaif]

Thank you @OmarIthawi !

Processes are created anyway by Celery with or without the fix. The problem is that this is not compatible with normal queues. JoinableQueue uses semaphores, I can't think of a data leak scenario because of that. And I don't think the server can run out of memory because of semaphores. But of course, I'm not saying this is guaranteed, we should bring it into action to verify it

When Celery forks the process; the queue in the thread is duplicated because it's a Python thread, not a real thread. JoinableQueue will guarantee queue object sharing

On the other hand, the problem with sync_mode is that it sends to segment directly with every single log. Blocking execution until sent, no thread is created, and also not batching logs at all

What should we do?

• take the possible risk?
• accept the sync_mode implications?
• do some changes to make Celery work with sync_mode but not edx-platform?
• accept the bug as unresolvable?

[OmarIthawi]

Thank you @OmarIthawi !

Processes are created anyway by Celery with or without the fix. The problem is that this is not compatible with normal queues. JoinableQueue uses semaphores, I can't think of a data leak scenario because of that. And I don't think the server can run out of memory because of semaphores. But of course, I'm not saying this is guaranteed, we should bring it into action to verify it

When Celery forks the process; the queue in the thread is duplicated because it's a Python thread, not a real thread. JoinableQueue will guarantee queue object sharing

On the other hand, the problem with sync_mode is that it sends to segment directly with every single log. Blocking execution until sent, no thread is created, and also not batching logs at all

What should we do?

• take the possible risk?
• accept the sync_mode implications?
• accept the bug as unresolvable?
• accept the sync_mode implications?

none are good alternatives in my opinion.

• do some changes to make Celery work with sync_mode but not edx-platform?

Yes but only in celery. The sync_mode makes total sense in celery and command line. To oversimplify the solution, we'd need the following form of client:

default_client = Client(write_key, ..., sync_mode=bool(settings.IS_CELERY))

or using a @task_postrun.connect like we did for honeycomb:

edx-platform/lms/celery.py

Lines 71 to 74 in 9baaffb

	@task_postrun.connect
	def end_celery_trace(task, state, **kwargs):
	beeline.add_field("celery.status", state)
	beeline.finish_trace(task.request.trace)

the idea is to clear the segment queue after each task run instead of never, this would work exactly like sync_mode but it may be easier to implement.

[bryanlandia]

@shadinaif @OmarIthawi Maybe a workaround to consider is to make an upstream PR to Segment, and in the meantime change our Celery config so that the queue used for any task that will generate an event via a worker not use multiple processes. I believe the only ones that do are PersistentGrade calculation and the Completion Aggregator tasks. We could create a new queue just for them and only allow 1 process.

Correct me if I'm wrong, but I believe it's the concurrency setting that controls the number of processes spawned. Right now, the default queue can have up to 8 processes!

https://github.com/appsembler/edx-configs/blob/master/appsembler/tahoe/us/juniper/prod/files/server-vars.yml#L350-L355

EDXAPP_CELERY_WORKERS:
    service_variant: lms
    concurrency: 8
    monitor: true
    prefetch_optimization: fair
    max_tasks_per_child: 1000

(abbreviated)

[OmarIthawi]

@shadinaif @OmarIthawi Maybe a workaround to consider is to make an upstream PR to Segment,

I think that's a great idea. As long as we're getting help on the review for the Queue changes we're fine.

and in the meantime change our Celery config so that the queue used for any task that will generate an event via a worker not use multiple processes. I believe the only ones that do are PersistentGrade calculation and the Completion Aggregator tasks. We could create a new queue just for them and only allow 1 process.

I'll let Shadi comment on this part.

Here are other alternatives that I'm proposing:

• flush and send segment events after each celery task #1174
• Celery sync mode #1173

[shadinaif]

Thank you @bryanlandia and @OmarIthawi . Looks like we have many good options. I see sync_mode for celery-only #1175 is the best option to start with. If it didn't work we'll choose another one

[shadinaif]

Closed in favour of #1175