sergehuber commented on code in PR #775:
URL: https://github.com/apache/unomi/pull/775#discussion_r3428516973
##########
plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java:
##########
@@ -239,9 +257,32 @@ public boolean eval(Condition condition, Item item,
Map<String, Object> context,
actualValue = ConditionContextHelper.foldToASCII((String)
actualValue);
}
+ final Object finalActualValue = actualValue;
+ if (tracer != null) {
+ tracer.trace("Property value comparison: " + name + " " + op +
" " + expectedValue,
+ new HashMap<String, Object>() {{
+ put("actualValue", finalActualValue);
+ put("expectedValue", expectedValue);
+ put("expectedValueInteger", expectedValueInteger);
+ put("expectedValueDouble", expectedValueDouble);
+ put("expectedValueDate", expectedValueDate);
+ put("expectedValueDateExpr", expectedValueDateExpr);
+ }});
Review Comment:
Valid advisory — double-brace initialization creates an anonymous subclass
per call on a hot evaluation path. Will be addressed in a follow-up to replace
it with explicit `Map` construction.
##########
plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/EventToProfilePropertyAction.java:
##########
@@ -21,20 +21,63 @@
import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.services.EventService;
+import org.apache.unomi.tracing.api.TracerService;
+import org.apache.unomi.tracing.api.RequestTracer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.HashMap;
/**
* A action to copy an event property to a profile property
*/
public class EventToProfilePropertyAction implements ActionExecutor {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(EventToProfilePropertyAction.class);
+ private TracerService tracerService;
+
+ public void setTracerService(TracerService tracerService) {
+ this.tracerService = tracerService;
+ }
public int execute(Action action, Event event) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (!tracer.isEnabled()) {
+ tracer.setEnabled(true);
+ }
Review Comment:
Fixed: Removed `tracer.setEnabled(true)`. Tracing is now opt-in — the action
only records trace data when `tracerService != null &&
tracerService.isTracingEnabled()`, matching the established pattern in
`ActionExecutorDispatcherImpl`. Same fix applied to
`EvaluateVisitPropertiesAction`, `CopyPropertiesAction`, and
`GroovyActionDispatcher`.
##########
plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/EvaluateVisitPropertiesAction.java:
##########
@@ -31,47 +33,97 @@
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.TimeZone;
+import java.util.HashMap;
/**
* This action is used to calculate the firstVisit, lastVisit and
previousVisit date properties on the profile
* Depending on the event timestamp it will adjust one or multiples of this
properties accordingly to the logical chronology.
*/
public class EvaluateVisitPropertiesAction implements ActionExecutor {
private static final Logger LOGGER =
LoggerFactory.getLogger(EvaluateVisitPropertiesAction.class.getName());
+ private TracerService tracerService;
+
+ public void setTracerService(TracerService tracerService) {
+ this.tracerService = tracerService;
+ }
public int execute(Action action, Event event) {
- SimpleDateFormat dateFormat = new
SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'");
- dateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (!tracer.isEnabled()) {
+ tracer.setEnabled(true);
+ }
Review Comment:
Fixed: Same guard applied — `tracerService.isTracingEnabled()` is checked
before any tracer calls, with no forced `setEnabled(true)`.
##########
plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/CopyPropertiesAction.java:
##########
@@ -37,49 +39,98 @@ public class CopyPropertiesAction implements ActionExecutor
{
private static final Logger LOGGER =
LoggerFactory.getLogger(CopyPropertiesAction.class);
private ProfileService profileService;
+ private TracerService tracerService;
public void setProfileService(ProfileService profileService) {
this.profileService = profileService;
}
+ public void setTracerService(TracerService tracerService) {
+ this.tracerService = tracerService;
+ }
+
@SuppressWarnings({ "unchecked", "rawtypes" })
public int execute(Action action, Event event) {
- boolean atLeastOnechanged = false;
- List<String> mandatoryPropTypeSystemTags = (List<String>)
action.getParameterValues().get("mandatoryPropTypeSystemTag");
- String singleValueStrategy = (String)
action.getParameterValues().get("singleValueStrategy");
- for (Map.Entry<String, Object> entry : getEventPropsToCopy(action,
event).entrySet()) {
- String mappedProperty = resolvePropertyName(entry.getKey());
-
- // propType Check
- PropertyType propertyType =
profileService.getPropertyType(mappedProperty);
- Object previousValue =
event.getProfile().getProperty(mappedProperty);
- if (mandatoryPropTypeSystemTags != null &&
mandatoryPropTypeSystemTags.size() > 0) {
- if (propertyType == null || propertyType.getMetadata() == null
|| propertyType.getMetadata().getSystemTags() == null
- ||
!propertyType.getMetadata().getSystemTags().containsAll(mandatoryPropTypeSystemTags))
{
- continue;
- }
- }
- String propertyName = "properties." + mappedProperty;
- boolean changed = false;
- if (previousValue == null && propertyType == null) {
- changed = PropertyHelper.setProperty(event.getProfile(),
propertyName, entry.getValue(), "alwaysSet");
- } else {
- boolean propertyTypeIsMultiValued =
- propertyType != null && propertyType.isMultivalued()
!= null && propertyType.isMultivalued();
- boolean multipleIsExpected = previousValue != null ?
previousValue instanceof List : propertyTypeIsMultiValued;
-
- if (multipleIsExpected) {
- changed = PropertyHelper.setProperty(event.getProfile(),
propertyName, entry.getValue(), "addValues");
- } else if (entry.getValue() instanceof List) {
- LOGGER.error("Impossible to copy the property of type List
to the profile, either a single value already exist on the profile or the
property type is declared as a single value property. Enable debug log level
for more information");
- LOGGER.debug("cannot copy property {}, because it's a
List", mappedProperty);
- } else {
- changed = PropertyHelper.setProperty(event.getProfile(),
propertyName, entry.getValue(), singleValueStrategy);
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (!tracer.isEnabled()) {
+ tracer.setEnabled(true);
+ }
Review Comment:
Fixed: `setEnabled(true)` removed; all tracer calls are now guarded behind
`tracerService != null && tracerService.isTracingEnabled()`.
##########
extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java:
##########
@@ -67,32 +73,54 @@ public void
setActionExecutorDispatcher(ActionExecutorDispatcher actionExecutorD
this.actionExecutorDispatcher = actionExecutorDispatcher;
}
+ @Reference
+ public void setTracerService(TracerService tracerService) {
+ this.tracerService = tracerService;
+ }
+
public String getPrefix() {
return GROOVY_PREFIX;
}
public Integer execute(Action action, Event event, String actionName) {
- Class<? extends Script> scriptClass =
groovyActionsService.getCompiledScript(actionName);
- if (scriptClass == null) {
- LOGGER.warn("Couldn't find a Groovy action with name {}, action
will not execute!", actionName);
- return 0;
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (!tracer.isEnabled()) {
+ tracer.setEnabled(true);
}
-
+
Review Comment:
Fixed: Same opt-in guard pattern applied. The dispatcher no longer forces
tracing on — it only traces when `tracerService.isTracingEnabled()` is true for
the current request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
