-
Notifications
You must be signed in to change notification settings - Fork 0
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
Cbang/simplify entry point2 #93
base: main
Are you sure you want to change the base?
Conversation
… implementing the AppInterface
…g testing of adherence to interface
… explicitly inherited from AppInterface using method resolution order
… been configured prior to use
…gSettings for test_logging_configuration
… new implementation of configure_logging
return _LOGGING_CONFIGURED | ||
|
||
|
||
class LoggingSettings(PydanticParsingSettings): |
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.
Sure this shouldn't just inherit from pydantic.BaseModel
?
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.
Yes, as we discussed with Bjarke that we want a generic Pydantic class where we control the most generic settings configurations, that meet our use cases with parameters and environment variables, such as:
cli_parse_args=True,
cli_kebab_case=True,
cli_ignore_unknown_args=True, (parsing will fail otherwise)
cli_implicit_flags=True (We have this some places)
extras: dict[str, Any] = {}, | ||
force_configuration: bool = False, | ||
logging_settings: LoggingSettings, | ||
extras: dict[str, Any] | None = None, |
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.
Why not just default this to an empty dict so you don't have to check for None later?
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.
You are thinking about the extras dict right? I agree that it should just default to None
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.
Be aware of the default args caveats:
https://medium.com/@nebiyuelias1/be-careful-when-using-default-arguments-in-python-fd92df94efee
@@ -214,3 +240,105 @@ def test__add_log_records_to_azure_monitor_keeps_correct_count( | |||
query=query, | |||
expected_count=log_count, | |||
) | |||
|
|||
|
|||
def test__decorators_integration_test( |
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.
Can you parameterize this so that these can be executed in parallel?
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.
There is a general delay before the logs show up in application insights, so when the first query succeeds, the remaining three queries find the results instantly - so I don't think it will improve the speed of things to parameterize 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.
We can do it for prettification though if you agree?
Description
This PR introduces a new start_trace() decorator which simplifies the setup of logging setup inside entry point files of business applications: e.g. in measurements or wholesale. Further, a LoggingSettings() pydantic settings class is used to handle the automatic parsing of cli parameters and environment variables.
An implementation of this functionality is avaialble in this PR in the measurements repo: Energinet-DataHub/opengeh-measurements#180
Files changed: