Skip to content
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

set up logging #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

set up logging #119

wants to merge 1 commit into from

Conversation

spolisar
Copy link

@spolisar spolisar commented Nov 14, 2024

Pull Request Template

TL;DR

  • Set up logging for streamlitanalytics2 properly with getLogger()
  • Used urllib3 as a reference for how logging should be set up in a library

Problem Solved

  1. When using streamlitanalytics2 and another library that has logging, info messages were being printed to the console when logging was not configured to do that.

Changes Made(Optional)

List the main changes in bullet points.

  • get a logger for the library and give it a null handler

Manual Testing

  • You have manually tested all changes to ensure they work as intended. Replace the [ ] with a [X] if True.

How was this tested?
Describe the tests that you ran to verify your changes.

  • Tested by running a streamlit app using the fork and a library with test logging INFO messages

Additional Notes

  • I used urllib3 and python's documentation as a reference for how logging should be configured in a library
  • If someone wants to access the logs in streamlit-analytics2 in an application using it they'd do something like this
import streamlit_analytics2
import logging

logger = logging.getLogger("streamlit_analytics2")
logger.setLevel(logging.DEBUG)

handler = logging.StreamHandler()
handler.setLevel(logging.DEBUG)
logger.addHandler(handler)

@spolisar spolisar requested a review from 444B as a code owner November 14, 2024 23:15
@444B
Copy link
Owner

444B commented Nov 18, 2024

Thanks for the PR! Im taking a look at this now

@444B 444B self-assigned this Nov 18, 2024
@444B
Copy link
Owner

444B commented Nov 18, 2024

@spolisar I love this PR, thank you for finding a great solution to the overwriting logging issue
I will have to play with this a bit more this week
If you can provide a full streamlit example that shows that it wont overwrite other logging configs then it will expedite the PR greatly
Thanks!

@spolisar
Copy link
Author

spolisar commented Nov 21, 2024

I wrote a simple example that replicates the issue I ran into, and led to this pull request, in this gist. main.py doesn't get squarelib's logger or set it up to display logs, so you'd expect no logs to appear in the terminal when running. That does not happen, we get log messages printed out in the console when running the streamlit app. If you comment out import streamlit_analytics2 and run the streamlit app, you won't see logs printed in the console. The changes in the pull request should fix that.

From what I understand about logging in python, there's a hierarchy/structure to loggers. Libraries like streamlit-analytics2, urllib3, or squarelib from the example should have their own logger that can fit into that hierarchy properly rather than using the root logging functions from the logging library.

Most of what I'm saying here is stuff I learned recently, so there may be mistakes.
I found this stack overflow post helpful in understanding how logging works in python, and led to the references to urllib3.

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