No longer send unnecessary identify calls from frontend #6464
Conversation
tconkling
left a comment
There was a problem hiding this comment.
Looks great! Needs a couple simple credentials.py tests - and we should also discuss whether we can remove (maybe in a separate PR) the "installation ID" and "email" properties from frontend code that currently uses them.
|
@tconkling I think I addressed all your suggestions -- let me know if there are other changes you would like me to make |
tconkling
left a comment
There was a problem hiding this comment.
Great, thanks Zachary! I left a few more nitpicky feedbacks; once those are fixed, let's merge it in!
(Also note to self/anyone else following along - I think we can remove email from UserInfo.proto after this is merged.)
|
Huzzah, ready to merge! @blackary do you have merge privs or should I hit the button? |
|
@tconkling Ain't got the power -- go for it! |
|
Just the usual reminder about double/triple/quadruple checking that this doesn't break anything in our telemetry. Because mistakes there live with us forever, as users continue to use old versions of Streamlit. Things to do: look at network traffic, at Segment traffic, at data coming into our backend, etc. And not just you, but have someone else look at it too. |
|
Thanks for the reminder, @sfc-gh-tteixeira -- let me do some more local testing with the whl |
| pythonVersion: environmentInfo.pythonVersion, | ||
| installationId: userInfo.installationId, | ||
| installationIdV3: userInfo.installationIdV3, | ||
| authorEmail: userInfo.email, |
There was a problem hiding this comment.
I think there are also a two other aspects on the python side that we can remove related to the email:
and
There was a problem hiding this comment.
@lukasmasuch I feel a bit less confident about being sure what all those touch (e.g. that would seem to affect app_session_test)-- if someone else would be willing to remove those in a separate PR, that would be appreciated
There was a problem hiding this comment.
@blackary yep, I can make the changes in a separate PR 👍
that would seem to affect app_session_test
You are probably referring to these user_info things here, or?
There is another user_info.email in our code, which is completely unrelated to this user_info.email 😅 I think most/all of the user user_info email uses are referring to the st.user feature, but it indeed makes this very confusing.
There was a problem hiding this comment.
Yes, that is what I was seeing, and assumed it was related 😂
lukasmasuch
left a comment
There was a problem hiding this comment.
LGTM as well 👍 I believe we can also remove two additional parts related to email (see comment above)
|
@tconkling @lukasmasuch I have tested this on my end and everything looks good, if someone would be willing to merge this in. |
* develop: Fix issue with deprecated asyncio test variable (streamlit#6517) No longer send unnecessary identify calls from frontend (streamlit#6464)
📚 Context
The Streamlit telemetry backend does not use the data that is being sent from the frontend through identifies events. This replaces these identifies calls with a single call.
Please describe the project or issue background here
What kind of change does this PR introduce?
🧠 Description of Changes
Add bullet points summarizing your changes here
Revised:
Insert screenshot of your updated UI/code here
Current:
Insert screenshot of existing UI/code here
🧪 Testing Done
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.