MA-157 Move context back into properties. #6172
Conversation
|
👍 |
|
Thanks @mulby |
|
K, I'll re-review then |
nasthagiri
force-pushed
the
mobile/update-analytics
branch
from
d89f9ca
to
f012f3d
Compare
|
@mulby I have updated this PR with the changes we discussed yesterday. Please re-review. |
|
@aleffert please take a look as well. |
| if ( | ||
| not segment_event_type or | ||
| (segment_event_type.lower() not in allowed_types) or | ||
| any(disallowed_subs_name in segment_event_name.lower() for disallowed_subs_name in disallowed_substring_names) |
There was a problem hiding this comment.
I would prefer we had a different error message for this since I see the wrong type and a filtered name as two very different problems.
In fact, we may not want to log any warnings for an ignored name since that will likely cause a lot of log noise.
We don't expect to receive events with an unhandled type very often.
There was a problem hiding this comment.
@mulby There will also be many events with types other than "track". In particular, all "screen" and "identity" event types are ignored and are listed in the logs.
There was a problem hiding this comment.
So should we not raise an error for disallowed Types as well?
There was a problem hiding this comment.
@mulby As discussed, we'll keep this as an error message for now, in case it will be helpful to debug any webhook issues on prod. We will remove this error message a few weeks after enabling on prod.
|
Once my one comment is addressed this looks good. 👍 |
|
👍 |
|
+1
|
|
@stroilova FYI. |
Move app-generated context back into properties. Ignore BI events through segment's webhook. Ignore events without Data in Properties.
nasthagiri
force-pushed
the
mobile/update-analytics
branch
from
4b4c542
to
a600399
Compare
|
The sole LMS acceptance failure is a known flaky test. Merging. |
MA-157 Move context back into properties.
https://openedx.atlassian.net/browse/MA-157
Here's the updated gist from android-generated video events:
https://gist.github.com/nasthagiri/58ec88c1a92e6641dd0e
Here's the updated gist from ios-generated video events:
https://gist.github.com/nasthagiri/d2ee58d93c90faae1b65
ARCHIVE (before BI blacklist and before android-app updates):
Here's a gist of the events in the event log: https://gist.github.com/nasthagiri/8d9e83734791cfc4950e