-
Notifications
You must be signed in to change notification settings - Fork 0
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
Harmonizing default Mixpanel properties across Rails & ingest (SCP-5798) #366
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #366 +/- ##
===============================================
+ Coverage 75.56% 75.66% +0.09%
===============================================
Files 30 30
Lines 4383 4392 +9
===============================================
+ Hits 3312 3323 +11
+ Misses 1071 1069 -2
|
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.
Code looks good! Useful observability refinement.
@@ -171,12 +175,15 @@ def study_file(self, study_file_id): | |||
self.file_type = self.study_file["file_type"] | |||
self.file_size = self.study_file["upload_file_size"] | |||
self.file_name = self.study_file["name"] | |||
upload_trigger = self.study_file.get("options", {}).get("upload_trigger") |
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.
Some day we'll get optional chaining in Python!
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 will make the Mixpanel data so much more useful! Thank you.
BACKGROUND && CHANGES
This update aims to harmonize some common Mixpanel event properties across
scp-ingest-pipeline
&single_cell_portal_core
. These include:action
: Main process being performed by ingest run, likeingest_cluster
ordifferential_expression
referenceAnnDataFile
: T/F indication if this is a "reference" AnnData upload (no data extracted/visualized)trigger
: this already existed inscp-ingest-pipeline
, but thebucket
value was never supported, meaning we did not know from ingest if this was an file that used the new "bucket path" featureThis will give us better insight into common Mixpanel events, especially for AnnData files. We can now see the specific action that failed and pair them with the
errorTypes
array, which contains more detailed error information than a simple exit code.MANUAL TESTING
ingest
directoryaction
listed now in the properties, which should show up asdifferential_expression
: