Skip to content

Improve logging - satosa.routing #275

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

Merged
merged 3 commits into from
Sep 19, 2019

Conversation

sebulibah
Copy link
Contributor

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@c00kiemon5ter
Copy link
Member

I'm taking a random change:

-        satosa_logging(logger, logging.DEBUG, "Routing to backend: %s " % context.target_backend, context.state)
+        logger.debug("Routing to backend: %s " % context.target_backend)

I note two things here:

  1. To concatenate strings we should prefer .format()
-        satosa_logging(logger, logging.DEBUG, "Routing to backend: %s " % context.target_backend, context.state)
+        msg = "Routing to backend: {backend}".format(backend=context.target_backend)
+        logger.debug(msg)
  1. The chosen change does not include context.state that was previously part of the content that was logged. Specifically, if we look at the satosa_logging() function, we see that state is only used to extract the session id. So, let's try to keep that:
-        satosa_logging(logger, logging.DEBUG, "Routing to backend: %s " % context.target_backend, context.state)
+        msg = "Routing to backend: {backend}".format(backend=context.target_backend)
+        logline = "[{id}] {message}".format(id=state.get("SESSION_ID"), message=msg)
+        logger.debug(logline)

@c00kiemon5ter c00kiemon5ter merged commit 4cfd932 into IdentityPython:master Sep 19, 2019
@c00kiemon5ter
Copy link
Member

c00kiemon5ter commented Sep 19, 2019

Thanks @sebulibah

This is the first merge towards improving the state of logging in the project. For those interested we will start by removing the satosa_logging function, replacing it with direct calls to the logger. Then, we will switch to using, probably, the structlog library and start taking advantage of the features of structured logging.

c00kiemon5ter added a commit that referenced this pull request Sep 19, 2019
@peppelinux
Copy link
Member

Good news, I prefer direct call to logging instances too

@c00kiemon5ter
Copy link
Member

c00kiemon5ter commented Sep 19, 2019

@sebulibah see 053df98 and 4065cfd .

I have extracted the log format and the way we should get the session id, so it should be easier to follow that pattern and avoid such typos in the future ;)

@sebulibah
Copy link
Contributor Author

Thanks @sebulibah

This is the first merge towards improving the state of logging in the project. For those interested we will start by removing the satosa_logging function, replacing it with direct calls to the logger. Then, we will switch to using, probably, the structlog library and start taking advantage of the features of structured logging.

You're welcome

@c00kiemon5ter c00kiemon5ter changed the title Improve current logging for SATOSA Improve current logging for SATOSA - satosa.routing Oct 10, 2019
@c00kiemon5ter c00kiemon5ter changed the title Improve current logging for SATOSA - satosa.routing Improve logging - satosa.routing Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants