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

Logging #86

Merged
merged 13 commits into from
Dec 10, 2019
Merged

Logging #86

merged 13 commits into from
Dec 10, 2019

Conversation

silverdaz
Copy link
Contributor

Correlation IDs are now included in the log record

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Code cleanup
  • Build system change
  • Documentation change
  • Language translation

Pull request long description:

The package __init__.py updates the logger class, and supplies one where we force correlation ids to be logged.
utils/ogging.py contains the new logger class, and a utility to find the correlation id in the context of the caller.
Unless the correlation id is itself specified in the log calls, we search up the call stack in the local variables of the caller and exits as soon as we find it, with a maximum of "levels" (initially fixed at 10, ie we search 10 callers' context up the stack). We return None otherwise.

Related issues:

Fixes #11

Additional information:

We swap the bootstrap default from s3 to posix. S3 was extremely slow to read (when about to decrypt). This is not the case for POSIX. We added an extra test for ingesting a 1GB file.

Mentions:

@viklund: You might like that solution. Code change in only 2 places.

@silverdaz silverdaz self-assigned this Dec 8, 2019
@silverdaz silverdaz requested a review from viklund December 9, 2019 11:28
Copy link
Contributor

@viklund viklund left a comment

Choose a reason for hiding this comment

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

Inspecting the stack to get the correlation id is somewhat hacky I think. But it'll do for now. As for the rest, I think it's fine.

The assertion for the correlation_id is unnecesary in the utils.amqp.publish function if it's just not given a default value in the parameters.

@silverdaz
Copy link
Contributor Author

silverdaz commented Dec 9, 2019

With commit #d5cb7bb, I remove the need for searching the call stack. I essentially stash the correlation id in something that acts as a global variable across the package and all the loggers use that variable. The key place for setting the correlation id is when we pick a message from rabbitmq. Remember that a python process is single-threaded, there is no real parallelism.

@silverdaz silverdaz merged commit 4041f34 into master Dec 10, 2019
@silverdaz silverdaz deleted the logging branch December 10, 2019 12:33
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.

Log with correlation_id throughout the ingestion process
2 participants