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
feat: support AuditLog and RequestLog protos #274
Conversation
tests/system/test_system.py
Outdated
logger.log_proto(audit_struct) | ||
|
||
# retrieve log | ||
filter_ = self.TYPE_FILTER.format(type_url) + f" AND {_time_filter}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a side note for logging language syntax and not to this specific line:
I find it ugly when we concatenate strings to form a query. Perhaps, we should think about creation of an abstraction that will make forming these filters more intuitive when you read the code.
For example:
Instead of
xxx + " AND " + time_filter
it could be
x.And(new BinaryOperator(time1, operands.Equal, time2)
This could be a separate task and not part of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting idea. I'm not sure if that specific example would be seen as idiomatic for Python, but there's probably something interesting we could do with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 cents: I recall in Python (and Node), this kind of interpolation is preferred
@@ -503,6 +503,20 @@ def test_to_api_repr_defaults(self): | |||
} | |||
self.assertEqual(entry.to_api_repr(), expected) | |||
|
|||
def test_to_api_repr_struct(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, no abbreviations in test or other method names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"to_api_repr" is an existing function that we can't change here without breaking the API. We could add this to the list for v3.0.0 changes (but it may not be worth the user confusion)
If you run
logger.list_logs()
and encounter an unknown proto log, it will crash the libraryThis PR adds support for the two officially supported proto types:
Fixes #16