Skip to content

feat: introduce log content on log records#170

Merged
AWS-Samuel merged 1 commit intoOpenJobDescription:mainlinefrom
AWS-Samuel:log-metadata
Sep 19, 2024
Merged

feat: introduce log content on log records#170
AWS-Samuel merged 1 commit intoOpenJobDescription:mainlinefrom
AWS-Samuel:log-metadata

Conversation

@AWS-Samuel
Copy link
Copy Markdown
Contributor

@AWS-Samuel AWS-Samuel commented Sep 12, 2024

What was the problem/requirement? (What/Why)

Consumers of the openjd_sessions module may want a means of filtering logs based on their contents.

What was the solution? (How)

In order to solve this problem, this commit introduces a structure we can add to the extra kwarg when logging messages. The value provided to the extra kwarg must be of type dict[str, Any], with the limitation that the keys cannot be an existing attribute name in LogRecord.

This commit then introduces a LogExtraInfo class which is a Dictionary containing one key: openjd_log_content.

openjd_log_content is a Flag which allows combining multiple LogContent values through bitwise operators in order to indicate which contents are in the logged message.

What is the impact of this change?

A LogContent flag becomes available to attached to logged messages, that consumers of this package can use to filter log messages based on the type of log message.

How was this change tested?

Have you run the unit tests?

Yes, also added unit tests to confirm all expected fields in log records still exist

Was this change documented?

It is through the changelog.

Is this a breaking change?

No

Does this change impact security?

No


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AWS-Samuel AWS-Samuel requested a review from a team as a code owner September 12, 2024 16:30
@ddneilson ddneilson self-requested a review September 12, 2024 16:40
Copy link
Copy Markdown
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

This is great, Sam. Thanks for taking this on.

Can you also add some public documentation for this so that users of the library can discover that this extra log metadata exists? Perhaps as a docstring on the LOG global in this file?

For example, we communicate that there's a session_id metadata in the log messages here.

Comment thread src/openjd/sessions/_logging.py Outdated
@ddneilson
Copy link
Copy Markdown
Contributor

Chatted offline. We'll also need to make sure that the uses of this don't clobber the extra that is added by the Session's LoggerAdaptor. All of the logger instances that are used in this library are actually instances of that object, so we'll need to make sure that it still works as expected.

@AWS-Samuel AWS-Samuel force-pushed the log-metadata branch 3 times, most recently from 8b6b758 to 6b608c4 Compare September 13, 2024 19:20
@AWS-Samuel AWS-Samuel changed the title feat: introduce log metadata feat: introduce log content on log records Sep 13, 2024
@AWS-Samuel
Copy link
Copy Markdown
Contributor Author

This is great, Sam. Thanks for taking this on.

Can you also add some public documentation for this so that users of the library can discover that this extra log metadata exists? Perhaps as a docstring on the LOG global in this file?

For example, we communicate that there's a session_id metadata in the log messages here.

Added, confirmed that the documentation shows up under hover text for LOG in my editor.

Chatted offline. We'll also need to make sure that the uses of this don't clobber the extra that is added by the Session's LoggerAdaptor. All of the logger instances that are used in this library are actually instances of that object, so we'll need to make sure that it still works as expected.

Confirmed that this was causing issue, so I added an override and added a test for this.

Copy link
Copy Markdown
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

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

One small nitpick. Thanks for fixing this up, Sam.

Comment thread src/openjd/sessions/_logging.py Outdated
Signed-off-by: Samuel Anderson <119458760+AWS-Samuel@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

@AWS-Samuel AWS-Samuel merged commit 6e98f16 into OpenJobDescription:mainline Sep 19, 2024
@AWS-Samuel AWS-Samuel deleted the log-metadata branch September 19, 2024 15:58
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