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

Fix parsing of the ini file to setup sqlalchemylogger #2297

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

ger-benjamin
Copy link
Member

@ger-benjamin ger-benjamin commented Jun 12, 2024

Hi @sbrunner

We are using this sqlalchemy logger in a project using c2cwsgiutils 1.4. It was working well.
And now, I try to upgrade to c2cwsgiutils 6.0.8, and I can't make it work.

This (draft) PR is open to discussion, and solves already somme issue related to the .ini file, and the way c2cwsgiutils parses it.

The example propose this config for the .ini file.:

[handler_sqlalchemy_logger]
class = c2cwsgiutils.sqlalchemylogger.handlers.SQLAlchemyHandler
args = ({'url':'postgresql://postgres:password@localhost:5432/test','tablename':'test','tableargs': {'schema':'xyz'}},'curl')
level = NOTSET
formatter = generic
propagate = 0

First, the args is wrong I think. I had to add a final comma, otherwise I get an error.

With the current code, the args here will become a stream property, like that:

stream: ext://({'url':'postgresql://postgres:password@localhost:5432/test','tablename':'test','tableargs': {'schema':'xyz'}},'curl')

It's valid for console, or JSON built-in handlers, but te sqlalchemy doesn't expect a stream. It expect a sqlalchem_url, a does_not_contain_expression and a contains_expression parameters.

I can't makes it work with the current python and the args (or kwargs) property. I've tried, even by chnaging the code and passing args instead of stream, but it doesn't work. So, I've adapted the code here to have class_args_ properties.
I think that's also more clear to use class_args_stream instead of args that becomes stream.

One other tweaks I add to do is to change the "handlers" parsing. The current code give returns a String with handlers: console, json. Now, it returns an array: handlers: ['console', 'json'].

With these change, I can starts to use the sqlalchemy parser. But here is still an issue between the emit and the process that write the log into the DB. But this part is magic, I'll need to discuss about this with you.

To do:

  • Validate change with you
  • Debug sqlalchemy writting => Not needed here: setup sqlalchemy in the project, not via the .ini
  • Change class_args_ to class_arg_ => use kwargs instead, and ban args
  • Update example's README
  • Add breaking change doc => not needed

New to do:

  • document pass stream as kwargs
  • document init of sqlalchemy logger, by app's main
  • Update .ini file

See also: https://docs.python.org/3/library/logging.config.html

@ger-benjamin ger-benjamin requested a review from sbrunner June 12, 2024 12:50
@ger-benjamin ger-benjamin self-assigned this Jun 12, 2024
@sbrunner
Copy link
Member

Why don't use the args instead of introducing custom class_args_* configuration?

@ger-benjamin
Copy link
Member Author

ger-benjamin commented Jun 12, 2024

I can't makes it work with the current python and the args (or kwargs) property. I've tried, even by changing the code and passing args instead of stream, but it doesn't work. So, I've adapted the code here to have class_args_ properties.
I think that's also more clear to use class_args_stream instead of args that becomes stream.

To be more clear, logging try to instantiate the c2cwsgiutils.handlers.sqlalchemy handler with only one argument: args (or even stream without my first adaptations), (as a string I guess). And not with sqlalchem_url, does_not_contain_expression and contains_expression arguments.

@ger-benjamin
Copy link
Member Author

Reading the code (and the doc) of logging, It would probably parse the args if it would be used as a fileConfig, but it's interpreted as a dictConfig. And I don't/understand how to do that.
But fileConfig looks more limited and older than dictConfig

@ger-benjamin ger-benjamin force-pushed the debug-sqlalchemylogger branch from ad72f4a to 56e1bd8 Compare June 14, 2024 11:46
@ger-benjamin
Copy link
Member Author

ger-benjamin commented Jun 14, 2024

Code updated using kwargs now.
Maybe I've to adapt the readme, to says explicitely how to setup the loggers via the python "main" of the project, and not from the ini.
I'll try first...

@ger-benjamin ger-benjamin force-pushed the debug-sqlalchemylogger branch 2 times, most recently from ee4ca39 to a7fdc19 Compare June 14, 2024 13:20
@ger-benjamin
Copy link
Member Author

ger-benjamin commented Jun 14, 2024

@sbrunner

Everything works now. And everythings' documented here I think.

I don't need a 6.1.0 release anymore for now.

I don't know the issue with the CI here.

@sbrunner
Copy link
Member

@ger-benjamin Thanks :-)

To fix the CI error, running pre-commit run black should resolve your issue :-)

Done for sqlalchemylogger, but actually the logger would try
to write logs (process) on another process than the logs
collecteur (emit) which can't work. The setup of sqlalchemy
logger must be done manually in the project and not via the
ini file.
@ger-benjamin ger-benjamin force-pushed the debug-sqlalchemylogger branch 2 times, most recently from c9a93d5 to 8cdd48e Compare June 17, 2024 06:46
@ger-benjamin ger-benjamin force-pushed the debug-sqlalchemylogger branch from 8cdd48e to ca728e7 Compare June 17, 2024 06:55
@ger-benjamin ger-benjamin marked this pull request as ready for review June 17, 2024 07:07
@ger-benjamin ger-benjamin merged commit 791785c into master Jun 17, 2024
7 checks passed
@ger-benjamin ger-benjamin deleted the debug-sqlalchemylogger branch June 17, 2024 07:07
@geo-ghci-int geo-ghci-int bot added this to the 6.1.0 milestone Oct 25, 2024
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.

2 participants