JCRVLT-848 adding warn before the node gets dropped as part of the im… - #428
JCRVLT-848 adding warn before the node gets dropped as part of the im…#428pat-lego wants to merge 2 commits into
Conversation
…port for better tracking of content import process
| getAclManagement().clearACL(node); | ||
| } | ||
| } else { | ||
| log.warn("Replace path: {}", child.getPath()); |
There was a problem hiding this comment.
Assume I have a package which removes a subtree with 1000 nodes, how frequently will this message be logged?
There was a problem hiding this comment.
Understandable I would say this could happen yes, but such large packages would also result in other issues such as heavy IO, which would lead me to believe this is not what you are going to worry about in that situation.
There was a problem hiding this comment.
I am rather worried about the noise in the logs it creates
There was a problem hiding this comment.
Then how can we capture this information in an intelligent way to help with observability because this is not the first occurence of an issue like this and the lack of observability creates issues that are complex to resolve.
| } | ||
|
|
||
| if (shouldRemoveChild) { | ||
| log.warn("Replacing path {}", path); |
There was a problem hiding this comment.
nitpick: I wonder if "replace" is the correct term; in the context of this change I see the term "remove" being used, which is a slightly different thing.
There was a problem hiding this comment.
Fair point I guess I saw it as "Replacing" because it may come back I know for now it is being removed but the filters may have this node in scope and so it could come back.
|
Also, do we really need "WARN" here? |
|
We do have two different ways of logging during package installation:
Who is supposed to see this message/warn? |
|
@kwin I think that the installation log itself contains that information already. But unless this installation is triggered via API and these "installation logs" are also dumped to the (slf4j) log, there is no trace that this happened. For example if you are using the AEM package manager, these deletes are logged to stdout; if you as a user don't store these logs -- normally no one is doing that -- there is no obvious track that this happened. But in this particular case it might be better to modify the package manager to store a copy of these logs "somewhere", instead of just logging some actions without further context. |
|
@reschke I felt that a node delete warrants a WARN especially if you are not anticipating it to be deleted but I am open to ideas just not DEBUG because then we cant do an RCA since the action will have already happened and most of the time the system is not in DEBUG mode. |
|
@kwin I agree with what @joerghoh is saying, often times users build the package (so its not installed) on Author and then replicate that package to the publishers (since most publishers do not offer a package manager). By doing this the content is effectively still on Author instances but missing on Publishers. Users are then confused as to what happened and have no visibility as to what caused this as no information is logged. Hence this PR aims to add some visibility as to the actions vault does during an installation process. |
|
|
|
Ive changed the level to INFO, addressed the verbiage used and I also noticed the file handler was not addressed. |
|
Logs could be throttled or information be summarized. |
|
How is modifying nodes via replication of packages fundamentally different from other ways. Again, why is audit logging not enough? |
|
Tail the audit.log auditlog.log file while you install a JCR package nothing is printed. |
|
I was referring to Oak's built in logging in https://github.com/apache/jackrabbit-oak/blob/c335cb6709c1ba83084a7a16d62fd2981a17fde1/oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/delegate/SessionDelegate.java#L730. That logs with DEBUG level by default (so maybe your application (AEM?) needs adjusting log levels), but that should IMHO cover that already. |
I see I had it in INFO which is why I was not seeing anything, these logs would be perfect to be honest, the issue is that these logs are not tracked and we never have it on DEBUG. The reason why it is not in DEBUG is because what @joerghoh has been referencing, I had time over the weekend to think of a way to implement this without requiring a lot of logging. For now I think we can close this out as I work on the implementation that does not require large volumes of logs to be generated. |
4 participants
JCRVLT-848 adding warn before the node gets dropped as part of the import for better tracking of content import process