Copilot commented on code in PR #775:
URL: https://github.com/apache/unomi/pull/775#discussion_r3425974036
##########
rest/src/main/java/org/apache/unomi/rest/endpoints/ContextJsonEndpoint.java:
##########
@@ -191,9 +213,17 @@ public ContextResponse contextJSONAsPost(ContextRequest
contextRequest,
contextResponse.setSessionId(sessionId);
}
+ // Add tracing information if requested
+ if (explain) {
+
contextResponse.setRequestTracing(tracerService.getTraceNode());
+ tracerService.getCurrentTracer().endOperation(null, "Context
request processed successfully");
+ }
Review Comment:
When `explain=true`, the root tracing operation is ended *after* the trace
tree is captured into the response. This can return a `TraceNode` without the
final endTime/description/result. End the operation first, then attach
`getTraceNode()` to the response.
##########
services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java:
##########
@@ -388,52 +391,71 @@ public void setSegmentDefinition(Segment segment) {
throw new BadSegmentConditionException();
}
} else {
+ // Start validation operation in tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("segment-condition-validation",
"Validating segment condition: " + segment.getItemId(), segment.getCondition());
+ }
+ }
+
if (segment.getMetadata().isEnabled()) {
TypeResolutionService typeResolutionService =
getTypeResolutionService();
if (typeResolutionService != null) {
typeResolutionService.resolveCondition("segments",
segment, segment.getCondition(), "segment " + segment.getItemId());
}
+ }
- if
(!persistenceService.isValidCondition(segment.getCondition(), new
Profile(VALIDATION_PROFILE_ID))) {
- throw new BadSegmentConditionException();
- }
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
+ List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(segment.getCondition());
- if (definitionsService.getConditionValidationService() !=
null) {
- List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(segment.getCondition());
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"segment-condition-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Segment validation completed with %d errors",
validationErrors.size()));
+ }
Review Comment:
The tracer operation result passed to `endOperation(...)` is inverted:
`!validationErrors.isEmpty()` reports success when there are errors. This makes
trace output misleading for explain/debug.
##########
rest/src/main/java/org/apache/unomi/rest/endpoints/EventsCollectorEndpoint.java:
##########
@@ -122,9 +139,17 @@ private EventCollectorResponse
doEvent(EventsCollectorRequest eventsCollectorReq
EventCollectorResponse response = new
EventCollectorResponse(eventsRequestContext.getChanges());
+ // Add tracing information if requested
+ if (explain) {
+ response.setRequestTracing(tracerService.getTraceNode());
+ tracerService.getCurrentTracer().endOperation(null, "Event
collection request processed successfully");
+ }
Review Comment:
When `explain=true`, the root tracing operation is ended *after* the trace
tree is captured into the response. This means the returned `TraceNode` may
miss the final description/result/endTime from `endOperation()`. End the
operation first, then call `getTraceNode()` and attach it to the response.
##########
services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java:
##########
@@ -677,10 +683,29 @@ protected void setRule(Rule rule, boolean
allowInvalidRules) {
}
}
-
if (rule.getCondition() != null) {
+ // Start validation operation in tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("rule-condition-validation",
"Validating rule condition: " + rule.getItemId(), rule.getCondition());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(rule.getCondition());
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"rule-condition-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Rule validation completed with %d errors",
validationErrors.size()));
+ }
Review Comment:
The tracer operation result passed to `endOperation(...)` is inverted:
`!validationErrors.isEmpty()` reports success when there are errors. This makes
explain traces misleading.
##########
services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java:
##########
@@ -188,8 +198,119 @@ public void setGoal(Goal goal) {
LOGGER.warn("Trying to save null goal, aborting...");
return;
}
- ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent(), "goal "+goal.getItemId()+" start event");
- ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent(), "goal "+goal.getItemId()+" start event");
+ boolean isValid = true;
+ if (goal.getStartEvent() != null) {
+ // Resolve condition type (skips parameter resolution - happens
on-demand in query builders/evaluators)
+ TypeResolutionService typeResolutionService =
getTypeResolutionService();
+ boolean startValid = typeResolutionService != null &&
typeResolutionService.resolveCondition("goals", goal, goal.getStartEvent(),
"goal "+goal.getItemId()+" start event");
+ isValid = isValid && startValid;
+ // Start validation operation in tracer for start event
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("goal-start-event-validation",
"Validating goal start event: " + goal.getItemId(), goal.getStartEvent());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
+ List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(goal.getStartEvent());
+
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"goal-start-event-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Goal start event validation completed with %d errors",
validationErrors.size()));
+ }
Review Comment:
The tracer operation result passed to `endOperation(...)` is inverted:
`!validationErrors.isEmpty()` reports success when there are errors. This makes
goal explain traces misleading.
##########
services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java:
##########
@@ -1072,7 +1101,7 @@ private void
recalculatePastEventOccurrencesOnProfiles(Condition eventCondition,
l.add(eventCondition);
- Integer numberOfDays =
PropertyHelper.getInteger(parentCondition.getParameter("numberOfDays"));
+ Integer numberOfDays = (Integer)
parentCondition.getParameter("numberOfDays");
Review Comment:
`numberOfDays` is cast to `Integer`, but it is sometimes provided as a
numeric string (see existing integration tests). This cast will throw
`ClassCastException` and break past-event segment recalculation. Use
`PropertyHelper.getInteger(...)` coercion instead.
##########
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:
This uses double-brace initialization (`new HashMap<>() {{ ... }}`), which
creates an extra anonymous class and can retain references unexpectedly. It
also adds allocation overhead inside a hot path (condition evaluation). Prefer
building a `Map` normally and passing it to `tracer.trace(...)`.
##########
persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java:
##########
@@ -204,17 +219,19 @@ private static Object parseParameterWithValidation(
* @param value the value to resolve
* @param scriptExecutor executor for script expressions
* @param parameterDefs optional parameter definitions for validation
+ * @param tracerService optional tracer service for warnings
* @param conditionTypeId condition type ID for context
* @param resolutionChain set of parameter keys/scripts already being
resolved (for cycle detection)
* @param depth current resolution depth
- * @return resolved value, or {@code RESOLUTION_ERROR} sentinel if cycle
detected or max depth exceeded
+ * @return resolved value, or null if cycle detected or max depth exceeded
*/
Review Comment:
The Javadoc says this method returns `null` when a cycle/max-depth is
detected, but the implementation returns the `RESOLUTION_ERROR` sentinel. This
mismatch can confuse future maintenance/debugging of parameter resolution.
##########
plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetEventOccurenceCountAction.java:
##########
@@ -47,85 +46,118 @@ public void setPersistenceService(PersistenceService
persistenceService) {
this.persistenceService = persistenceService;
}
+ public void setTracerService(TracerService tracerService) {
+ this.tracerService = tracerService;
+ }
+
@Override
public int execute(Action action, Event event) {
- final Condition pastEventCondition = (Condition)
action.getParameterValues().get("pastEventCondition");
-
- Condition andCondition = new
Condition(definitionsService.getConditionType("booleanCondition"));
- andCondition.setParameter("operator", "and");
- ArrayList<Condition> conditions = new ArrayList<Condition>();
-
- Condition eventCondition = (Condition)
pastEventCondition.getParameter("eventCondition");
- definitionsService.resolveConditionType(eventCondition);
- conditions.add(eventCondition);
+ RequestTracer tracer = null;
+ if (tracerService != null && tracerService.isTracingEnabled()) {
+ tracer = tracerService.getCurrentTracer();
+ tracer.startOperation("set-event-count",
+ "Setting event occurrence count", action);
+ }
- Condition c = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
- c.setParameter("propertyName", "profileId");
- c.setParameter("comparisonOperator", "equals");
- c.setParameter("propertyValue", event.getProfileId());
- conditions.add(c);
+ try {
+ final Condition pastEventCondition = (Condition)
action.getParameterValues().get("pastEventCondition");
+ String generatedPropertyKey = (String)
pastEventCondition.getParameter("generatedPropertyKey");
- // may be current event is already persisted and indexed, in that case
we filter it from the count to increment it manually at the end
- Condition eventIdFilter = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
- eventIdFilter.setParameter("propertyName", "itemId");
- eventIdFilter.setParameter("comparisonOperator", "notEquals");
- eventIdFilter.setParameter("propertyValue", event.getItemId());
- conditions.add(eventIdFilter);
+ if (tracer != null) {
+ tracer.trace("Processing event count", Map.of(
+ "propertyKey", generatedPropertyKey,
+ "eventId", event.getItemId()
+ ));
+ }
- Integer numberOfDays = (Integer)
pastEventCondition.getParameter("numberOfDays");
- String fromDate = (String) pastEventCondition.getParameter("fromDate");
- String toDate = (String) pastEventCondition.getParameter("toDate");
+ Condition andCondition = new
Condition(definitionsService.getConditionType("booleanCondition"));
+ andCondition.setParameter("operator", "and");
+ ArrayList<Condition> conditions = new ArrayList<Condition>();
- if (numberOfDays != null) {
- Condition numberOfDaysCondition = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
- numberOfDaysCondition.setParameter("propertyName", "timeStamp");
- numberOfDaysCondition.setParameter("comparisonOperator",
"greaterThan");
- numberOfDaysCondition.setParameter("propertyValueDateExpr", "now-"
+ numberOfDays + "d");
- conditions.add(numberOfDaysCondition);
- }
- if (fromDate != null) {
- Condition startDateCondition = new Condition();
-
startDateCondition.setConditionType(definitionsService.getConditionType("eventPropertyCondition"));
- startDateCondition.setParameter("propertyName", "timeStamp");
- startDateCondition.setParameter("comparisonOperator",
"greaterThanOrEqualTo");
- startDateCondition.setParameter("propertyValueDate", fromDate);
- conditions.add(startDateCondition);
- }
- if (toDate != null) {
- Condition endDateCondition = new Condition();
-
endDateCondition.setConditionType(definitionsService.getConditionType("eventPropertyCondition"));
- endDateCondition.setParameter("propertyName", "timeStamp");
- endDateCondition.setParameter("comparisonOperator",
"lessThanOrEqualTo");
- endDateCondition.setParameter("propertyValueDate", toDate);
- conditions.add(endDateCondition);
- }
+ Condition eventCondition = (Condition)
pastEventCondition.getParameter("eventCondition");
+ if (eventCondition != null) {
+
definitionsService.getConditionValidationService().validate(eventCondition);
+ }
+ conditions.add(eventCondition);
+
Review Comment:
`eventCondition` may be null, but it is unconditionally added to
`subConditions`. This will later cause NPEs when the boolean condition is
evaluated / translated into a query. If `eventCondition` is missing, this
action should fail fast (or skip execution) instead of adding a null
subcondition.
##########
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:
This dispatcher forcibly enables tracing (`tracer.setEnabled(true)`) even
when tracing was not requested. Because tracers are ThreadLocal, this can
unintentionally keep tracing enabled for later requests handled by the same
thread. Tracing should be opt-in (only when `TracerService.isTracingEnabled()`
is true) and not enabled from inside action dispatchers.
##########
plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetEventOccurenceCountAction.java:
##########
@@ -47,85 +46,118 @@ public void setPersistenceService(PersistenceService
persistenceService) {
this.persistenceService = persistenceService;
}
+ public void setTracerService(TracerService tracerService) {
+ this.tracerService = tracerService;
+ }
+
@Override
public int execute(Action action, Event event) {
- final Condition pastEventCondition = (Condition)
action.getParameterValues().get("pastEventCondition");
-
- Condition andCondition = new
Condition(definitionsService.getConditionType("booleanCondition"));
- andCondition.setParameter("operator", "and");
- ArrayList<Condition> conditions = new ArrayList<Condition>();
-
- Condition eventCondition = (Condition)
pastEventCondition.getParameter("eventCondition");
- definitionsService.resolveConditionType(eventCondition);
- conditions.add(eventCondition);
+ RequestTracer tracer = null;
+ if (tracerService != null && tracerService.isTracingEnabled()) {
+ tracer = tracerService.getCurrentTracer();
+ tracer.startOperation("set-event-count",
+ "Setting event occurrence count", action);
+ }
- Condition c = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
- c.setParameter("propertyName", "profileId");
- c.setParameter("comparisonOperator", "equals");
- c.setParameter("propertyValue", event.getProfileId());
- conditions.add(c);
+ try {
+ final Condition pastEventCondition = (Condition)
action.getParameterValues().get("pastEventCondition");
+ String generatedPropertyKey = (String)
pastEventCondition.getParameter("generatedPropertyKey");
- // may be current event is already persisted and indexed, in that case
we filter it from the count to increment it manually at the end
- Condition eventIdFilter = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
- eventIdFilter.setParameter("propertyName", "itemId");
- eventIdFilter.setParameter("comparisonOperator", "notEquals");
- eventIdFilter.setParameter("propertyValue", event.getItemId());
- conditions.add(eventIdFilter);
+ if (tracer != null) {
+ tracer.trace("Processing event count", Map.of(
+ "propertyKey", generatedPropertyKey,
+ "eventId", event.getItemId()
+ ));
+ }
- Integer numberOfDays = (Integer)
pastEventCondition.getParameter("numberOfDays");
- String fromDate = (String) pastEventCondition.getParameter("fromDate");
- String toDate = (String) pastEventCondition.getParameter("toDate");
+ Condition andCondition = new
Condition(definitionsService.getConditionType("booleanCondition"));
+ andCondition.setParameter("operator", "and");
+ ArrayList<Condition> conditions = new ArrayList<Condition>();
- if (numberOfDays != null) {
- Condition numberOfDaysCondition = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
- numberOfDaysCondition.setParameter("propertyName", "timeStamp");
- numberOfDaysCondition.setParameter("comparisonOperator",
"greaterThan");
- numberOfDaysCondition.setParameter("propertyValueDateExpr", "now-"
+ numberOfDays + "d");
- conditions.add(numberOfDaysCondition);
- }
- if (fromDate != null) {
- Condition startDateCondition = new Condition();
-
startDateCondition.setConditionType(definitionsService.getConditionType("eventPropertyCondition"));
- startDateCondition.setParameter("propertyName", "timeStamp");
- startDateCondition.setParameter("comparisonOperator",
"greaterThanOrEqualTo");
- startDateCondition.setParameter("propertyValueDate", fromDate);
- conditions.add(startDateCondition);
- }
- if (toDate != null) {
- Condition endDateCondition = new Condition();
-
endDateCondition.setConditionType(definitionsService.getConditionType("eventPropertyCondition"));
- endDateCondition.setParameter("propertyName", "timeStamp");
- endDateCondition.setParameter("comparisonOperator",
"lessThanOrEqualTo");
- endDateCondition.setParameter("propertyValueDate", toDate);
- conditions.add(endDateCondition);
- }
+ Condition eventCondition = (Condition)
pastEventCondition.getParameter("eventCondition");
+ if (eventCondition != null) {
+
definitionsService.getConditionValidationService().validate(eventCondition);
+ }
+ conditions.add(eventCondition);
+
+ Condition c = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
+ c.setParameter("propertyName", "profileId");
+ c.setParameter("comparisonOperator", "equals");
+ c.setParameter("propertyValue", event.getProfileId());
+ conditions.add(c);
+
+ // may be current event is already persisted and indexed, in that
case we filter it from the count to increment it manually at the end
+ Condition eventIdFilter = new
Condition(definitionsService.getConditionType("eventPropertyCondition"));
+ eventIdFilter.setParameter("propertyName", "itemId");
+ eventIdFilter.setParameter("comparisonOperator", "notEquals");
+ eventIdFilter.setParameter("propertyValue", event.getItemId());
+ conditions.add(eventIdFilter);
+
+ Integer numberOfDays = (Integer)
pastEventCondition.getParameter("numberOfDays");
Review Comment:
`numberOfDays` is cast to `Integer`, but it can be provided as a numeric
string (existing tests cover this). This cast can throw `ClassCastException`
and break event occurrence counting. Use `PropertyHelper.getInteger(...)`
coercion instead.
##########
services/src/main/java/org/apache/unomi/services/impl/definitions/DefinitionsServiceImpl.java:
##########
@@ -573,9 +580,25 @@ public boolean resolveConditionType(Condition
rootCondition) {
// Delegate to TypeResolutionService for resolution
boolean resolved =
typeResolutionService.resolveConditionType(rootCondition, "condition type " +
rootCondition.getConditionTypeId());
if (resolved) {
+ // Start validation operation in tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("condition-validation", "Validating
condition: " + rootCondition.getConditionTypeId(), rootCondition);
+ }
+ }
+
// Validate the condition after resolving its type (validation
service will auto-resolve if needed)
List<ValidationError> validationErrors =
conditionValidationService.validate(rootCondition);
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"condition-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Validation completed with %d errors", validationErrors.size()));
+ }
Review Comment:
The tracer operation result passed to `endOperation(...)` is inverted:
`!validationErrors.isEmpty()` reports success when there are errors, making
explain traces misleading.
##########
services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java:
##########
@@ -188,8 +198,119 @@ public void setGoal(Goal goal) {
LOGGER.warn("Trying to save null goal, aborting...");
return;
}
- ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent(), "goal "+goal.getItemId()+" start event");
- ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent(), "goal "+goal.getItemId()+" start event");
+ boolean isValid = true;
+ if (goal.getStartEvent() != null) {
+ // Resolve condition type (skips parameter resolution - happens
on-demand in query builders/evaluators)
+ TypeResolutionService typeResolutionService =
getTypeResolutionService();
+ boolean startValid = typeResolutionService != null &&
typeResolutionService.resolveCondition("goals", goal, goal.getStartEvent(),
"goal "+goal.getItemId()+" start event");
+ isValid = isValid && startValid;
+ // Start validation operation in tracer for start event
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("goal-start-event-validation",
"Validating goal start event: " + goal.getItemId(), goal.getStartEvent());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
+ List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(goal.getStartEvent());
+
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"goal-start-event-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Goal start event validation completed with %d errors",
validationErrors.size()));
+ }
+ }
+
+ // Separate errors and warnings
+ List<ValidationError> errors = validationErrors.stream()
+ .filter(error -> error.getType() !=
ValidationErrorType.MISSING_RECOMMENDED_PARAMETER)
+ .collect(Collectors.toList());
+
+ List<ValidationError> warnings = validationErrors.stream()
+ .filter(error -> error.getType() ==
ValidationErrorType.MISSING_RECOMMENDED_PARAMETER)
+ .collect(Collectors.toList());
+
+ // Log warnings but don't block the operation
+ if (!warnings.isEmpty()) {
+ StringBuilder warningMessage = new StringBuilder("Goal start
event has warnings:");
+ for (ValidationError warning : warnings) {
+ warningMessage.append("\n- ").append(warning.getMessage());
+ }
+ LOGGER.warn(warningMessage.toString());
+ }
+
+ // Only throw exception for actual errors
+ if (!errors.isEmpty()) {
+ StringBuilder errorMessage = new StringBuilder("Invalid goal
start event:");
+ for (ValidationError error : errors) {
+ errorMessage.append("\n- ").append(error.getMessage());
+ }
+ if (typeResolutionService != null) {
+ typeResolutionService.markInvalid("goals",
goal.getItemId(), "Start event validation errors: " + errorMessage.toString());
+ }
+ throw new IllegalArgumentException(errorMessage.toString());
+ }
+ }
+ if (goal.getTargetEvent() != null) {
+ // Resolve condition type (skips parameter resolution - happens
on-demand in query builders/evaluators)
+ TypeResolutionService typeResolutionService =
getTypeResolutionService();
+ boolean targetValid = typeResolutionService != null &&
typeResolutionService.resolveCondition("goals", goal, goal.getTargetEvent(),
"goal "+goal.getItemId()+" target event");
+ isValid = isValid && targetValid;
+ // Start validation operation in tracer for target event
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("goal-target-event-validation",
"Validating goal target event: " + goal.getItemId(), goal.getTargetEvent());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
+ List<ValidationError> targetValidationErrors =
definitionsService.getConditionValidationService().validate(goal.getTargetEvent());
+
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(targetValidationErrors,
"goal-target-event-validation");
+ tracer.endOperation(!targetValidationErrors.isEmpty(),
String.format("Goal target event validation completed with %d errors",
targetValidationErrors.size()));
+ }
Review Comment:
The tracer operation result passed to `endOperation(...)` is inverted:
`!targetValidationErrors.isEmpty()` reports success when there are errors. This
makes goal explain traces misleading.
##########
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:
This action forcibly enables tracing (`tracer.setEnabled(true)`) even when
tracing was not requested. Since tracers are ThreadLocal, this can accidentally
keep tracing enabled across later requests on the same thread. Tracing should
be opt-in (only execute when `TracerService.isTracingEnabled()` is true) and
should not be enabled from inside an action.
##########
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:
This action forcibly enables tracing (`tracer.setEnabled(true)`) even when
tracing was not requested. Since tracers are ThreadLocal, this can
unintentionally keep tracing enabled for later requests on the same thread.
Tracing should be opt-in (only run when `tracerService.isTracingEnabled()` is
true) and never enabled from inside an action.
##########
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:
This action forcibly enables tracing (`tracer.setEnabled(true)`) even when
tracing was not requested. Because the tracer is stored in a ThreadLocal, this
can accidentally keep tracing enabled across subsequent requests handled by the
same thread, increasing overhead and potentially leaking trace data into
unrelated admin explain responses. Tracing should be opt-in (only when
`TracerService.isTracingEnabled()` is true) and never enabled from inside an
action.
--
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]
