Skip to content
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

feat: add tracking logs #173

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andrey-canon
Copy link
Contributor

@andrey-canon andrey-canon commented Dec 4, 2023

Description:
This changes allow to emit events when an aggregator is created or updated, and basically I'm proposing this to integrate those new events with the event-routing-backends library, which takes advantages of the tracking events to publish that data to a Xapi or caliper service. The default behavior that I'm proposing has the following events

Progress events, those will be emitted when the block has not reached the 100% of completion

  • edx.completion_aggregator.progress.course
  • edx.completion_aggregator.progress.chapter
  • edx.completion_aggregator.progress.sequential
  • edx.completion_aggregator.progress.vertical

Completion events, those will be emitted when the block has reached the 100% of completion

  • edx.completion_aggregator.completion.course
  • edx.completion_aggregator.completion.chapter
  • edx.completion_aggregator.completion.sequential
  • edx.completion_aggregator.completion.vertical

Testing instructions:

  1. Create a section with a subsection and a unit
  2. Complete the unit
  3. Check server logs

image

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

@andrey-canon andrey-canon force-pushed the and/add_tracking_logs branch from d0f60e3 to ead8f61 Compare December 4, 2023 22:14
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for preparing this PR, @andrey-canon!

@@ -81,5 +96,7 @@ def root(*args):
]
USE_TZ = True

EVENT_TRACKING_ENABLED = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is unused. Should we use it to gate this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used by the eventtracking library, in the test scenario I'm implementing the default django_tracker that is added by this AppConfig, that uses this method to register the tracker, after checking the DJANGO_ENABLED_SETTING_NAME which is EVENT_TRACKING_ENABLED as you can see in this line

Comment on lines 198 to 211
tracker.emit(
event_name,
{
"user_id": obj.user_id,
"course_id": str(obj.course_key),
"block_id": str(obj.block_key),
"modified": obj.modified,
"created": obj.created,
"earned": obj.earned,
"possible": obj.possible,
"percent": obj.percent,
"type": event_type,
}
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the cost of this operation (especially when we use multiple event-tracking backends)? Is it possible to emit multiple events in bulk?

I want to be particularly cautious about this, as we had faced a partial outage before due to the high volume of additional operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After running a basic test in my local I got the following
image

As you can see the time is in the order of 0.002 seconds, that test was performed with the following EVENT_TRACKING_BACKENDS value

{'tracking_logs': {'ENGINE': 'eventtracking.backends.routing.RoutingBackend',
  'OPTIONS': {'backends': {'logger': {'ENGINE': 'eventtracking.backends.logger.LoggerBackend',
     'OPTIONS': {'name': 'tracking', 'max_event_size': 50000}}},
   'processors': [{'ENGINE': 'common.djangoapps.track.shim.LegacyFieldMappingProcessor'},
    {'ENGINE': 'common.djangoapps.track.shim.PrefixedEventProcessor'}]}},
 'segmentio': {'ENGINE': 'eventtracking.backends.routing.RoutingBackend',
  'OPTIONS': {'backends': {'segment': {'ENGINE': 'eventtracking.backends.segment.SegmentBackend'}},
   'processors': [{'ENGINE': 'eventtracking.processors.whitelist.NameWhitelistProcessor',
     'OPTIONS': {'whitelist': []}},
    {'ENGINE': 'common.djangoapps.track.shim.GoogleAnalyticsProcessor'}]}},
 'xapi': {'ENGINE': 'eventtracking.backends.async_routing.AsyncRoutingBackend',
  'OPTIONS': {'backend_name': 'xapi',
   'processors': [{'ENGINE': 'eventtracking.processors.whitelist.NameWhitelistProcessor',
     'OPTIONS': {'whitelist': ['edx.problem.completed',
       'showanswer',
       'edx.completion.block_completion.changed',
       'edx.grades.problem.submitted',
       'edx.course.enrollment.activated',
       'edx.course.enrollment.deactivated',
       'edx.course.grade.passed.first_time',
       'edx.course.grade.now_passed',
       'edx.course.grade.now_failed',
       'load_video',
       'edx.video.loaded',
       'play_video',
       'edx.video.played',
       'stop_video',
       'edx.video.stopped',
       'complete_video',
       'edx.video.completed',
       'pause_video',
       'edx.video.paused']}}],
   'backends': {'xapi': {'ENGINE': 'event_routing_backends.backends.events_router.EventsRouter',
     'OPTIONS': {'processors': [{'ENGINE': 'event_routing_backends.processors.xapi.transformer_processor.XApiProcessor',
        'OPTIONS': {}}],
      'backend_name': 'xapi'}}}}},
 'caliper': {'ENGINE': 'eventtracking.backends.async_routing.AsyncRoutingBackend',
  'OPTIONS': {'backend_name': 'caliper',
   'processors': [{'ENGINE': 'eventtracking.processors.whitelist.NameWhitelistProcessor',
     'OPTIONS': {'whitelist': ['edx.course.enrollment.activated',
       'edx.course.enrollment.deactivated',
       'edx.ui.lms.link_clicked',
       'edx.ui.lms.sequence.outline.selected',
       'edx.ui.lms.outline.selected',
       'edx.ui.lms.sequence.next_selected',
       'edx.ui.lms.sequence.previous_selected',
       'edx.ui.lms.sequence.tab_selected',
       'showanswer',
       'edx.problem.hint.demandhint_displayed',
       'problem_check',
       'load_video',
       'edx.video.loaded',
       'play_video',
       'edx.video.played',
       'complete_video',
       'edx.video.completed',
       'stop_video',
       'edx.video.stopped',
       'pause_video',
       'edx.video.paused',
       'seek_video',
       'edx.video.position.changed',
       'edx.course.grade.passed.first_time',
       'edx.course.grade.now_passed',
       'edx.course.grade.now_failed']}}],
   'backends': {'caliper': {'ENGINE': 'event_routing_backends.backends.events_router.EventsRouter',
     'OPTIONS': {'processors': [{'ENGINE': 'event_routing_backends.processors.caliper.transformer_processor.CaliperProcessor',
        'OPTIONS': {}},
       {'ENGINE': 'event_routing_backends.processors.caliper.envelope_processor.CaliperEnvelopeProcessor',
        'OPTIONS': {'sensor_id': 'https://localhost:18000'}}],
      'backend_name': 'caliper'}}}}}}

If I'm not wrong tracking_logs and segmentio comes by default, xapi and caliper are added by the event-routing-backends library.

Finally I'm not aware of a bulk emit capability, I couldn't find anything related to that in the eventtracking library and all the implementation that I found are using the single emit method

Comment on lines 16 to 27
"progress": [
"course",
"chapter",
"sequential",
"vertical",
],
"completion": [
"course",
"chapter",
"sequential",
"vertical",
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"progress": [
"course",
"chapter",
"sequential",
"vertical",
],
"completion": [
"course",
"chapter",
"sequential",
"vertical",
]
"progress": {
"course",
"chapter",
"sequential",
"vertical",
},
"completion": {
"course",
"chapter",
"sequential",
"vertical",
}

event = "progress" if obj.percent < 1 else "completion"
event_type = obj.aggregation_name

if event_type not in settings.ALLOWED_COMPLETION_AGGREGATOR_EVENT_TYPES.get(event, []):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if event_type not in settings.ALLOWED_COMPLETION_AGGREGATOR_EVENT_TYPES.get(event, []):
if event_type not in settings.ALLOWED_COMPLETION_AGGREGATOR_EVENT_TYPES.get(event, {}):

@Agrendalath
Copy link
Member

@andrey-canon, I will review and test this PR next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants