-
Notifications
You must be signed in to change notification settings - Fork 6
Merge records instead of overwrite and updates to metric portfolio #137
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
base: main
Are you sure you want to change the base?
Merge records instead of overwrite and updates to metric portfolio #137
Conversation
…fined at instantiation
…metrics Essentially harmonize the approach to session-state-level and aggregate-level metrics recording
|
Updated original PR description to encompass a bug I discovered with session-level stats that was easy enough to fix in this fairly small PR as well. |
|
Hey @444B just checking in. Do you have everything you need to review? Would love to get this pushed through (if you don't see any issues ofc) so I can install in my parent project and reap the benefits. |
|
Hi @emigre459 ! |
…2 instances in firestore
|
Sorry to keep packing stuff in, but just realized that we need per_day widget states for our analytics use case so added those too. If it's too much in a single PR, no worries, can split it up. |
… vars already present for today
|
Hi
|
|
We actually discussed some of this earlier when I pushed individual session tracking, but for our use case we provide typical cookie accept/reject dialogue. Similarly a user management screen in the parent app can handle data deletion requests that could remove firestore records. I guess the question is: do you want SA2 to be the piece of the tech stack that manages that? We could probably build a data deletion function in SA2 if you like, with the caveat of course that it would still be up to individual devs to use it. |
…ssuming it's there)
I went ahead and just pushed a Do you think it makes sense to add a data privacy section to the README or as its own document or some such? We could provide some code snippets there for people to leverage to do things like a cookie banner for opt-in and pointing out the session data deletion functionality. |
|
@444B just checking to see if you think the data deletion option provided may satisfy your concerns? Happy to push a mod if you think there's a better way. |
Description
Update firestore.save() to a merge, instead of overwrite, action to preserve custom data elements (e.g. adding a username to track individual user session analytics for a registered user). Also discovered a bug with session-level tracking that was incorrectly resetting pageviews and total time passed that I was able to fix via refactor.
As it was quick to do, I also added a "session_time_seconds" metric that is recorded with the other daily metrics, so devs could see how long the typical session for a user at the day level (and thus aggregate it up to longer time periods as needed).
Related Issue(s)
Issue exists in forked repo but not parent.
Changes Made
merge=Truekwarg to the.set()operations infirestore.save()session_datainst.session_statebut use it as a quasi-global variable similar to what is done withdatato harmonize tracking approach.session_time_secondsto theper_daygroup in analytics records to expand analysisper_daylist and update both new per_day fields to be backwards compatible with existing firestore recordsTesting
Testing Details:
Tested
examples/minimal.pyby modifying thefirebase_test.pypage to use my google firebase credentials and project.streamlit-analyticscollection withcountsrecordcountsdocument ("username": "johnsmith")usernameentry did not disappear (which was the previous behavior)Screenshots (if applicable)
N/A
Checklist
Additional Notes
Had to run
uv pip install types-tomlto avoid failures fromrun_checks.shbut no changes were observed in git as a result.