Skip to content

feat: flask logging dashboard#66

Draft
ishandhanani wants to merge 10 commits intomasterfrom
log-dashboard
Draft

feat: flask logging dashboard#66
ishandhanani wants to merge 10 commits intomasterfrom
log-dashboard

Conversation

@ishandhanani
Copy link
Contributor

rubi-py PR

  • Note - please make sure the title of your PR follows semantic versioning
  • Ex. fix/feat/refactor: description
  • For a breaking change, that will bump an entire version, please use !. Ex. refactor!: description

Description

Logs are great but they're hard to sift through and are difficult to work with

What was the issue?

rubi-py users have requested a persistent dashboard that is display logs as they come in

What were the changes?

Added a basic flask app that displays logs on localhost:5000 if turned on

What type of change was this

  • fix - fixing bugs and adding small changes (X.X.X+1)
  • feat - introducing a new feature (X.X+1.X)
  • breaking - a breaking API change (X+1.X.X)

@ishandhanani ishandhanani changed the title feat: basic flask logging dashboard feat: flask logging dashboard Jul 17, 2023
Comment on lines +20 to +22
url = "http://localhost:5000/api/logs" # Update the URL if necessary
response = requests.post(url, json={"logs": logs})
print(response.text)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key here. This sends a POST request to the Flask endpoint.

I'm trying to figure out the most efficient way to use this across the repo. Basically, every time that something is logged, it should call get_logs and also POST it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea - I can create a custom class APILogHandler as a subclass of logging.Handler and create a new emit function that also posts it to the Flask API.

In each file that requires logging, you can then import from logging_config import logger and then use the same logger command

I'm not sure how this would affect normal logging when the front end is not being used. I could add logic that checks if the Flask port is active. If yes, it posts otherwise it behaves like a normal log statement.

Would like to get your thoughts on this @adamamsmith and @denverbaumgartner before I implement in terms of design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the idea of extending logging.Handler for use throughout the repo. i think we can provide this as an optional flag/setting somewhere that indicates the user would like to post logs to a url if desired. by default, lets have it configured to behave like a normal log statement. we could store the flask api (localhost:5000) in the network_config and allow the user to update this if they so choose.

another path here could be utilizing something like OpenTelemetry #4. this may be more than we need right now, but from my understanding we can instrument the repo to take advantage of a lot of existing tooling.

final thoughts here, we can take advantage of more granular logging as well to provide more clarity to what is going (debug, warning, etc.) in the repo as is today

Copy link
Contributor

@adamamsmith adamamsmith Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ishandhanani I think creating a custom logHandler is a must for this. This means you don't have to go and add a whole bunch of code in order to log to this dashboard but can rather just add the custom log handler when you want to run the dash.

And then all you would need to do is have an if statement based on if you want to add the dash log handler and then everything else will work as intended (and the terminal logs will still be output in addition to the dash).

Note that you can add global log handlers like this:

level=logging.INFO

logging.basicConfig(
            format="%(asctime)s - %(name)s - %(levelname)s - %(message)s",
            level=level,
            handlers=[
                logging.StreamHandler(sys.stdout),
                customDashHandler,
            ],

Which then means you won't have to set the config in every file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On @denverbaumgartner's point another must is to go through the logs currently in the repo and make sure that all the logs being emitted are at a sensible level.



# Function to retrieve the captured logs
def get_logs():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we should do something like this here to clear out the logs:

def get_logs():
logs = log_stream.getvalue().strip().split("\n")
log_stream.seek(0)
log_stream.truncate()
return logs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline, there may be more elegant ways to do this that doesn't clear memory in case we want to access those logs later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed get_logs. Instead, the log behavior can now be edited through the app.py. Currently, when new logs are detected, the logs list is extended with new_logs. I don't think this prints them twice. But this behavior can be customized

api_enabled = True


class APILogHandler(logging.Handler):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the api_enabled flag is hardcoded to true but this implementation currently prints to the Flask dashboard and to the command line

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.

3 participants