Skip to content

Conversation

@emigre459
Copy link

@emigre459 emigre459 commented Mar 2, 2025

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

  • Add merge=True kwarg to the .set() operations in firestore.save()
  • Refactored to no longer store session_data in st.session_state but use it as a quasi-global variable similar to what is done with data to harmonize tracking approach.
  • Add session_time_seconds to the per_day group in analytics records to expand analysis
  • Add widget states to the per_day list and update both new per_day fields to be backwards compatible with existing firestore records

Testing

  • I have manually tested these changes and confirm they work as intended.

Testing Details:
Tested examples/minimal.py by modifying the firebase_test.py page to use my google firebase credentials and project.

  1. Created streamlit-analytics collection with counts record
  2. Used a small script to add a new key-value pair to the counts document ("username": "johnsmith")
  3. Queried the document in firestore to confirm addition of key-value pair
  4. Performed click action in testing streamlit app
  5. Confirmed that click event counter increased and username entry did not disappear (which was the previous behavior)
  6. Refreshed page a few times to force incrementing of pageviews and observed the incrementing occurring for both aggregate stats (already working) as well as session-level stats (which were not working and constantly resetting to 0/1).
  7. Setup firebase_test.py (did not commit changes) to use a testing collection in firestore and confirm that existing records could be updated for new per_day tracking without causing errors

Screenshots (if applicable)

N/A

Checklist

  • I have read the CONTRIBUTING guide.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation (README.md, CONTRIBUTING.md, etc.).
  • My changes generate no new warnings or errors.

Additional Notes

Had to run uv pip install types-toml to avoid failures from run_checks.sh but no changes were observed in git as a result.

@emigre459 emigre459 requested a review from 444B as a code owner March 2, 2025 02:18
…metrics

Essentially harmonize the approach to session-state-level and aggregate-level metrics recording
@emigre459
Copy link
Author

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.

@emigre459 emigre459 changed the title 2 feature track widget interactions by key not label Merge records instead of overwrite and updates to metric portfolio Mar 3, 2025
@emigre459
Copy link
Author

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.

@444B
Copy link
Owner

444B commented Mar 5, 2025

Hi @emigre459 !
So sorry for the delay, I haven't had a chance to review these
I hope to find some time this weekend to do so
Thank you for your patience 🙏

@emigre459
Copy link
Author

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.

@444B
Copy link
Owner

444B commented Mar 11, 2025

Hi
if you are looking to implement a feature that tracks individual user session analytics,
How is the following handled:

  1. user consent and
  2. data deletion requests

@emigre459
Copy link
Author

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.

@emigre459
Copy link
Author

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.

I went ahead and just pushed a delete_session_data() function in main.py to address your second concern. I realized after my last message that it would probably lower the energy barrier for app devs to be more proactive.

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.

@emigre459
Copy link
Author

@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.

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