[MGNLDAM-531] DamTemplatingFunctions spams logs with useless error messages Created: 17/Dec/14  Updated: 10/Feb/15  Resolved: 02/Feb/15

Status: Closed
Project: Magnolia DAM Module
Component/s: None
Affects Version/s: 1.2.7
Fix Version/s: 1.2.8

Type: Improvement Priority: Major
Reporter: Richard Unger Assignee: Christoph Meier
Resolution: Fixed Votes: 0
Labels: quickwin, requires_documentation, support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
relation
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Description   

Please remove the log statements that produce the following output:

WARN info.magnolia.dam.asset.functions.DamTemplatingFunctions 16.12.2014 15:55:37 – AssetIdentifier is null or blank, return Null.
WARN info.magnolia.dam.asset.functions.DamTemplatingFunctions 16.12.2014 15:55:37 – No Asset found for assetIdentifier , return Null.
WARN info.magnolia.dam.asset.functions.DamTemplatingFunctions 16.12.2014 15:55:37 – No Asset found for assetIdentifier , return Null link.

This is completely useless in a production environment.

1) Please reduce the log priority to DEBUG, not WARN

2) Please include in the debug message the identifier that can't be found (if not null), or even better, include the current URI from the aggregation state as well.



 Comments   
Comment by Christoph Meier [ 08/Jan/15 ]

Hello Richard could you please provide the Magnolia and or DAM version you are using when this log messages appear.
Thanks in advance.

Comment by Christoph Meier [ 08/Jan/15 ]

This issue is not part of DAM 2.0.x but of DAM 1.2.x.
So this would be something for the 5.2.10 changelog

info.magnolia.dam.asset.functions.DamTemplatingFunctions exists on the DAM 1.2.x branch.
On the 2.0.x branch of DAM, DamTemplatingFunctions has been moved to info.magnolia.dam.templating.functions.

So i guess propose the fix-version here must be 1.2.8

Comment by Christoph Meier [ 08/Jan/15 ]

Changes committed onto branch "MGNLDAM-531" (which derived from 1.2.x branch).

For me it looks like this must not be ported to 2.0.x and master branch.
Since 2.0, DamTemplatingFunctions has been moved and the appropriate methods are implemented differently.

Comment by Magnolia International [ 23/Jan/15 ]

If you're relying on debug logs to figure out those issues (which the patch indicates), then configure log4j to output the current uri. This patch introduces a dependency onto MgnlContext's static current context, which is not desirable here. Please revert.

Comment by Christoph Meier [ 02/Feb/15 ]

@Richard Unger
To log current URI, please configure log4j accordingly.
For an example see example patch.
If you wonder where these values come from, see ContextFilter

Comment by Christoph Meier [ 03/Feb/15 ]

Log level has been standardized for all methods. WARN for caught exceptions; DEBUG when returning null.
And now all public methods (again) will log, although in some of the common use cases this will lead to 2 log entries for one missing "item". Since these methods are public, all of them should log.

Comment by Richard Unger [ 10/Feb/15 ]

Thanks for the fix for 5.2.10. We will install and test it.

However, I think the 5.3 series will be affected by the same problems? It seems to me the getItem() functions all log WARNING messages, and don't differentiate by exception type.
Even worse, the getRendition() method logs a warning if the asset can't be found.

Please also change this warning to debug...

Comment by Richard Unger [ 10/Feb/15 ]

@Gregory: Thanks for the feedback. I don't quite see the problem with using the MgnlContext object - that's what it is for, isn't it.

I agree you need to be defensive, as there may not be an active WebContext, but this can be checked quite easily: ( ctx = MgnlContext.getWebContextOrNull(); if (ctx!=null) ... )

I will think about adding the URI to the log4j logs, that is a useful idea! Compared to adding it only to this log statement it seems a bit heavy-weight though...

Generated at Mon Feb 12 05:00:46 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.