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

EZP-26593: Enable audit log by default #1267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peterkeung
Copy link
Contributor

https://jira.ez.no/browse/EZP-26593

This feature should be enabled by default: https://doc.ez.no/eZ-Publish/Technical-manual/4.x/Features/Audit-trailing

  • We eventually enable the audit log for most clients
  • The audit log usually gets enabled after something bad has happened (and at that point we waste a bunch of time trying to analyze the Apache logs)
  • There are only positives to having the audit log enabled
  • The log files automatically rotate, so there isn't additional configuration that we need to remember

@crevillo
Copy link
Contributor

I'm not fully agree with this. Looking to, for example, symfony does in term of loggin, by default it tries to reduce the amount of logs it creates in prod environments. I would prefer to not have this enabled by default. Besides, is easier to activate on your own.

But pinging @emodric @wizhippo to know their opinions.

@wizhippo
Copy link
Contributor

I'm with @crevillo on this one. This is a feature that I believe should not be on by default.

@emodric
Copy link
Collaborator

emodric commented Nov 11, 2016

Agreed with @crevillo and @wizhippo! It's useful, but should not be on by default.

@peterkeung
Copy link
Contributor Author

Over the years I have found content hide, move, and delete history to be just as core as edit history. That's the main argument. Sure, it's easy to enable but catches people by surprise that it's not default.

@thiagocamposviana
Copy link
Contributor

As long as there is a rotation by default I don't see a big harm on enabling it by default, to the opposite, sometimes we found ourselves asking to check the audit logs to see who deleted a certain content just to find out it was not enabled and we couldn't properly "audit" the issue.

@gggeek
Copy link
Contributor

gggeek commented Nov 12, 2016

I'm in favour of having it on by default - the audit log is rotated, and the IO you get out of it is small anyway.

My main concern with it in fact is that it is not omni-comprehensive, and that it should log more operations. All operations except content publication should be logged: move/hide/set-state/set-section/trash/swap-nodes/add-or-remove-location/change-sorting/change-priority etc... (without the need to set up up dedicated workflow events)

Btw, is this even a feature in eZ5? Maybe we should have a budle for that?

Side note : @crevillo Sf does many things different than eZ by default/principle. The funny thing is: quite a few of them make lots of sense for a framework, but less for a CMS. Eg. the whole "throw your arms in the air if there is the smallest error anywhere" works fine for code and configuration, but not for code that touches content, as content always ends up inconsistent (and eZ had to turn a lot of screws to reintroduce graceful handling of those cases in version 5, instead of 'show blank page'. Heck, proper handling of bad object relations is still being worked on these days...)

@crevillo
Copy link
Contributor

Looks we need a github feature to add polls here, but even though, i know can count 3 "yes" and 3 "no". :).

Who should we ping to break the tie here?

@dougplant
Copy link
Contributor

What is the case for not having it be default behaviour? Thus far, we're looking at reducing the amount of log files created which is a secondary consideration here.

@crevillo
Copy link
Contributor

I don't know what could be the reason to have it off by default. But looks like nobody though in having it enabled by default until now :).

Looks we have 4 yes and 3 no now. We can merge this, but we should ping the eZ doc team to change the doc @peterkeung pointed in the link where it says it's disabled by default.

Also i guess we should doc this is a BC change. Somebody can have a own audit doing whatever maybe needed and probably adding more overhead than the already existing already messages. And maybe he/she just want to have it enabled from time to time. We should warn user about this will be enabled by default.

@emodric @wizhippo ok to merge?

@crevillo
Copy link
Contributor

crevillo commented Nov 13, 2016

damn, did i merge this by mistake?
Edit: Sorry. looks its merged in the mugoweb repo, not here.

Besides, it looks i don't have access to merge this, even i thought i had...

@emodric
Copy link
Collaborator

emodric commented Nov 13, 2016

I'm still -1 for this change since it can be considered as a breaking change, due to it being a change in default settings.

I don't see a problem of keeping the config enabled in your overrides if you really need it. It's config, not code.

@andrerom
Copy link
Contributor

Same here, if there was some new major/minor release planned and thus corresponding doc change it could make sense. But as there is not this is not something we'd put into a patch release, which master effectively is here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants