logging improvements - Githubissues

collective / pas.plugins.ldap

Zope (and Plone) PAS Plugin providing users and groups from LDAP directory

http://pypi.python.org/pypi/pas.plugins.ldap

Other

13 stars 19 forks source link

logging improvements #82

Closed jensens closed 4 years ago

jensens commented 4 years ago

Log LDAP-errors as level error, to get them i.e. into Sentry. In fact these are errors.
Make timeout of LDAP-errors logging configurable with environment variable PAS_PLUGINS_LDAP_ERROR_LOG_TIMEOUT.
Log long running LDAP/ pas.plugin.ldap operations as error. Threshold can be controlled with environment variable PAS_PLUGINS_LDAP_LONG_RUNNING_LOG_THRESHOLD.

jensens commented 4 years ago

Good point, the operation timeouts can not be influenced by now. The first is really to no flood logs. There is also a retry and retrydelay possible to configure. We may want to have this all as environment variables. Or should we better expose those in the UI as well?

fredvd commented 4 years ago

We may want to have this all as environment variables. Or should we better expose those in the UI as well?

@jensens +1 to add the values to the UI. We ran into a related issue when we wanted to configure the memcached server address from ENV, thats why Maurits created an alternative ICacheSettingsRecordProvider which picks up an environment setting for the address if it is in the env in collective.ldapsetup:

https://github.com/collective/collective.ldapsetup/blob/34c98ee6d0039a336e3bac5e0f23e53e17f0470b/collective/ldapsetup/cache.py#L28-L34

But the UI doesn't show you (yet) it is overriden. We could merge that small feature from collective.ldapsetup in the main pas.plugins. ldap as well now that we are talking environment vars ;-)

I know 0.01 YAFOWIL: can you on startup 'ghost' a YAFOWIL field in Python when it's been passed into pas.plugins.ldap by via env?

jensens commented 4 years ago

can you on startup 'ghost' a YAFOWIL field in Python when it's been passed into pas.plugins.ldap by via env?

Yes, YAFOWIL forms are just data structures, which can be modified at runtime. This gets a bit out of scope here IMO. What about merging this change first and then prepare another PR with enhanced environment variable handling?

fredvd commented 4 years ago

@jensens Depends on the scope. ;-) I was just probing a bit on how you look at adding these and other runtime/deployment/extra variables in both env and UI. There seems to be a supported path, so that's cool.

If you merge l could install a .dev release on some test environments to check for funny AD in the wild timeouts/logs/errors etc.

mauritsvanrees commented 4 years ago

Okay, I have merged it. Thanks Jens!