444B / streamlit-analytics2

👀 Track & visualize user interactions with your streamlit app
MIT License
14 stars 2 forks source link

[BUG] `state_dict` is reset every time `start_tracking()` is called #100

Closed robino16 closed 1 week ago

robino16 commented 1 week ago

Description There is a typo in the start_tracking function where the state_dict is incorrectly checked as state_dic, causing the state_dict to be reset every time start_tracking is called.

Expected behavior The state_dict should be stored in the session state for the duration of the session without being reset. This will ensure counts are correctly updated.

To Reproduce

Additional context This bug is also present in the original streamlit-analytics repo by jrieke.

Suggested fix

# Current incorrect code
if "state_dic" not in st.session_state:
    st.session_state.state_dict = {}

# Suggested fix
if "state_dict" not in st.session_state:
    st.session_state.state_dict = {}
444B commented 1 week ago

@robino16 thank you very much for raising this issue!

I will take a look at this today and test a version with the typo fixed.

Do you by any chance have a streamlit python script that you used to validate the dic getting wiped? I want to understand the before and after state when calling the function twice and I am curious how this was found

444B commented 1 week ago

Testing package available on testpypi https://test.pypi.org/project/streamlit-analytics2/ Once fix is validated, will push a main release

444B commented 1 week ago

Hi @robino16 I have fixed the typo however this has opened pandoras box TLDR: you can use start_tracking multiple times but you can only use stop_tracking once

It would seem that streamlit-analytics2 now correctly allow the multiple usage of start_tracking but it doesnt allow the multiple usage of stop_tracking since this function calls the display.show_results function. https://github.com/444B/streamlit-analytics2/blob/f475e60ef83549361eb6c6775e63d41e0a2fabbe/src/streamlit_analytics2/main.py#L545C9-L545C68

Calling display.show_results twice leads to duplicate keys for the selectbox for the reset prompt since that gets called on Screenshot 2024-06-29 144856

I believe that resolved the issue but I am still curious about the use case for calling start_tracking multiple times if you can only call stop_tracking once.

Ill allow a few hours for discussion on this topic before marking as resolved and pushing the typo and dep fix to main package. Feel free to test the current test package as shared in previous comment

robino16 commented 1 week ago

Hi, @444B .

In my case, I had a multi-page Streamlit app where I wanted to track analytics data individually for each page. Since the counts dict is shared across the pages, I had to modify the main script and adapt it to my use case. I came across this small typo while doing this. I also have noticed that the count values don't always update correctly, but I am not sure this typo is the only culprit here. I just wanted to let you know, since this repo is still being maintained.

The start_tracking function gets called every time a user performs a action in the UI, and since state_dic is never stored in the Streamlit session state, the st.session_state.state_dict = {} will be run on every user action, resetting the state_dict. This means that counts, e.g., for the checked checkboxes, increases on every user action, even if the value of the checkbox remains unchanged. I believe this is unintended behavior.

I believe fixing this typo will resolve some of these issues. Thank you! I won't be able to conduct any tests until later today, sorry.

444B commented 1 week ago

@robino16 Of course! Thank you for raising the matter and please always raise a bug for any issue or feature.
I take pride in maintaining this project and although I wish I had more time to devote to it, I will always resolve the bugs that I can and keep deps up to date.

Ill push out 0.7.7 now which has the typo fix. I have tested this and it seems to work.
Please keep me in the loop via this issue #100 if you have an issue with the typo issue specifically

robino16 commented 1 week ago

To test this change, I made a quick Streamlit app with two checkboxes and a button. I then did the same sequence of actions with and without the typo:

  1. I first checked the checkbox with label "Enable Cats".
  2. Then I clicked the button with label "Click me!" three times.

Note: Performing this sequence of actions will result in the script being rerun 5 times in total (one when launching the app, then four extra times for every user interactions). This means that the start_tracking function will be called 5 times as well.

// here is what we would expect the counts to be:
{"Enable Cats": 1, "Enable Dogs": 0, "Click me!": 3}

Here are the counts that were captured during my tests:

// with the typo:
{"Enable Cats": 5, "Enable Dogs": 5, "Click me!": 3}
// after fixing the typo:
{"Enable Cats": 2, "Enable Dogs": 1, "Click me!": 3}

Without the typo fix, we see that all the checkboxes have count 5 which is the same number of times as the script was run. Altough, only the "Enable Cats" checkbox was checked, we see no difference between the counts of the two checkboxes.

After implementing the fix, we see that "Enable Cats" have count 2 which is one more than the "Enable Dogs" checkbox. The extra count was triggered when I checked the checkbox, which does make sense and which is an improvement.

We can thus conclude that fixing the typo solves one problem.

However, there is still a bug here. Since I only clicked the "Enable Cats" checkbox once, I would expect it to store the value 1 and 0 for "Enable Dogs. This is due to a different bug with the _wrap_checkbox wrapper where the counts for checkboxes are incremented on startup, before any action has been done by the user. I will report this in a separate issue.