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

Configure logging #54

Merged
merged 8 commits into from
Nov 9, 2023
Merged

Configure logging #54

merged 8 commits into from
Nov 9, 2023

Conversation

tg2414
Copy link
Contributor

@tg2414 tg2414 commented Oct 26, 2023

Description

The init.py file has been updated to match datahub/init.py.

The log_config.py file has been added to match datahub/core/log_config.py

Close #51

Further checks

  • from the standup on wednesday, references to uvicorn may need to change

Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro left a comment

Choose a reason for hiding this comment

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

pre-commit QA is failing because there is no newline at the end of the file. Add this in for now, but the easiest way to avoid this is to install pre-commit into the virtual environment you're using locally (it is part of the development requirements).

It might be as simple as doing a find-replace from uvicorn -> gunicorn to get that working correctly.

app/__init__.py Outdated Show resolved Hide resolved
app/__init__.py Outdated Show resolved Hide resolved
@AdrianDAlessandro
Copy link
Collaborator

I would like you to update all the print statements you can find in the code with either log.debug or log.info

Specifically, do this in app/pages/control.py. Then you should be able to navigate to http://127.0.0.1:8050/control/ and click on the buttons and see the message appear in the logs on the command line

- 8050:8050
volumes:
- ./app:/app
- ./logs:/logs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, I didn't think properly when I asked you to add this in. You'll have to revert the commit that added this file. The docker-compose.yml isn't tracked by git because it is generated by configure.py so instead you will have to add a line to that python script to include this logs volume. Find where the app volume is added and add it that way. Test if it has worked by running the configure.py script

@AdrianDAlessandro AdrianDAlessandro changed the title Added in some of the solution from Gridlington-datahub #98. Configure logging Nov 8, 2023
Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro left a comment

Choose a reason for hiding this comment

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

I've still gotta get you to revert the commit that aded the docker-compose file in. Easiest way would be to run git revert 808cddf

configure.py Outdated
@@ -76,7 +76,7 @@ def generate_docker_compose(template_file: str, ip: str, develop: bool = False)
}
if develop:
docker_compose["services"]["dash"]["build"] = "."
docker_compose["services"]["dash"]["volumes"] = ["./app:/app"]
docker_compose["services"]["dash"]["volumes"] = ["./app:/app", "./logs:/logs"]
Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro Nov 9, 2023

Choose a reason for hiding this comment

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

Ok, we're almost there. These logs need to be included in the production version as well as the develop version, which means it needs to be added to where docker_compose["services"]["dash"] is defined above this if develop block. But only the logs volume and not the app volume.

I'd suggest adding the something like "volumes": ["./logs:/logs"] to the dictionary above and then changing this line to:

Suggested change
docker_compose["services"]["dash"]["volumes"] = ["./app:/app", "./logs:/logs"]
docker_compose["services"]["dash"]["volumes"] += ["./app:/app"]

…velop version, then the app volume will be added as well
@AdrianDAlessandro AdrianDAlessandro merged commit 16dfde5 into main Nov 9, 2023
4 checks passed
@AdrianDAlessandro AdrianDAlessandro deleted the log_config branch November 9, 2023 15:32
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.

Implement formal logging
2 participants