UNOMI-873: Add request tracing and web-servlets extraction#775
Merged
Conversation
Introduce tracing-api/tracing-impl for the Explain feature, wire TracerService across services, evaluators, actions, and REST endpoints, and extract legacy HTTP servlets into the web-servlets module while preserving master review fixes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new request-tracing capability (API + implementation) intended to power an “Explain” feature across Unomi’s request processing pipeline, and extracts legacy HTTP servlets from the WAB into a dedicated web-servlets bundle registered via Karaf features.
Changes:
- Added
tracing-api/tracing-implmodules withTracerService,RequestTracer, andTraceNode, plus an initial unit test suite. - Wired tracing into key execution paths (services, condition evaluation, actions, schema validation, and REST endpoints via
?explain=true). - Moved legacy servlets into a new
web-servletsmodule and updated Karaf feature wiring/config.
Reviewed changes
Copilot reviewed 65 out of 66 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| web-servlets/src/main/resources/org.apache.unomi.web.cfg | New configuration file for legacy servlet bundle. |
| web-servlets/src/main/java/org/apache/unomi/web/servlets/WebConfig.java | Package adjusted for extracted servlet module. |
| web-servlets/src/main/java/org/apache/unomi/web/servlets/HttpUtils.java | Package adjusted for extracted servlet module. |
| web-servlets/src/main/java/org/apache/unomi/web/servlets/HttpServletRequestForwardWrapper.java | Package adjusted for extracted servlet module. |
| web-servlets/src/main/java/org/apache/unomi/web/servlets/EventsCollectorServlet.java | Migrated servlet to OSGi HTTP Whiteboard DS registration. |
| web-servlets/src/main/java/org/apache/unomi/web/servlets/ContextServlet.java | Migrated servlet to OSGi HTTP Whiteboard DS registration. |
| web-servlets/src/main/java/org/apache/unomi/web/servlets/ClientServlet.java | Migrated servlet to OSGi HTTP Whiteboard DS registration. |
| web-servlets/pom.xml | New bundle module definition for legacy servlets and cfg attachment. |
| wab/pom.xml | Removed legacy servlet-related dependencies and cfg attachment from WAB. |
| tracing/tracing-impl/src/test/java/org/apache/unomi/tracing/impl/DefaultTracerServiceTest.java | New unit tests for tracing implementation. |
| tracing/tracing-impl/src/main/java/org/apache/unomi/tracing/impl/LoggingRequestTracer.java | Logging tracer variant with indentation support. |
| tracing/tracing-impl/src/main/java/org/apache/unomi/tracing/impl/DefaultTracerService.java | Default OSGi TracerService implementation. |
| tracing/tracing-impl/src/main/java/org/apache/unomi/tracing/impl/DefaultRequestTracer.java | Thread-local trace tree implementation + safe context rendering. |
| tracing/tracing-impl/pom.xml | New tracing implementation bundle module. |
| tracing/tracing-api/src/main/java/org/apache/unomi/tracing/api/TracerService.java | New tracing service API. |
| tracing/tracing-api/src/main/java/org/apache/unomi/tracing/api/TraceNode.java | New trace tree node model. |
| tracing/tracing-api/src/main/java/org/apache/unomi/tracing/api/RequestTracer.java | New tracer interface. |
| tracing/tracing-api/pom.xml | New tracing API bundle module. |
| tracing/pom.xml | New aggregator module for tracing. |
| services/src/main/resources/OSGI-INF/blueprint/blueprint.xml | Injected TracerService into core service beans. |
| services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java | Added tracing + refactored condition validation/type resolution paths. |
| services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java | Added tracing around rule validation/resolution. |
| services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java | Added tracing around goal condition validation/resolution. |
| services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java | Added tracing for event send flow + condition validation hooks. |
| services/src/main/java/org/apache/unomi/services/impl/definitions/DefinitionsServiceImpl.java | Added tracing around deprecated resolveConditionType validation flow. |
| services/src/main/java/org/apache/unomi/services/actions/impl/ActionExecutorDispatcherImpl.java | Added tracing + action type resolution adjustments. |
| services/pom.xml | Added tracing API dependency (and impl for tests). |
| rest/src/main/java/org/apache/unomi/rest/models/EventCollectorResponse.java | Added requestTracing field to REST response model. |
| rest/src/main/java/org/apache/unomi/rest/endpoints/EventsCollectorEndpoint.java | Added explain param and tracing enable/attach logic. |
| rest/src/main/java/org/apache/unomi/rest/endpoints/ContextJsonEndpoint.java | Added explain param and tracing enable/attach logic; updated sanitization helper. |
| rest/pom.xml | Added tracing API dependency. |
| pom.xml | Registered new tracing and web-servlets modules in the reactor. |
| plugins/baseplugin/src/main/resources/OSGI-INF/blueprint/blueprint.xml | Injected TracerService into evaluators/actions; added new config property. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java | Added tracing around property condition evaluation. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/NotConditionEvaluator.java | Added tracing around NOT evaluator. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/IdsConditionEvaluator.java | Added tracing around IDs evaluator. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/BooleanConditionEvaluator.java | Added tracing around boolean evaluator. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/UpdatePropertiesAction.java | Added tracing around updateProperties action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetPropertyAction.java | Added tracing around setProperty action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetEventOccurenceCountAction.java | Added tracing + refactored condition validation; builds boolean query condition. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SendEventAction.java | Added tracing around sendEvent action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/ModifyConsentAction.java | Added tracing around modifyConsent action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/MergeProfilesOnPropertyAction.java | Added tracing around mergeProfiles action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java | Added tracing around incrementProperty action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/EventToProfilePropertyAction.java | Added tracing around event-to-profile-property action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/EvaluateVisitPropertiesAction.java | Added tracing around evaluate-visit-properties action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/EvaluateProfileSegmentsAction.java | Added tracing and loop-prevention handling when invoked on profileUpdated. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/EvaluateProfileAgeAction.java | Added tracing and loop-prevention handling when invoked on profileUpdated. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/CopyPropertiesAction.java | Added tracing around copyProperties action. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/AllEventToProfilePropertiesAction.java | Added tracing around deprecated action. |
| plugins/baseplugin/pom.xml | Added tracing API dependency. |
| persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImpl.java | Added tracing around condition evaluation and contextualization. |
| persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java | Added tracer-aware parameter validation warnings + new overload. |
| persistence-spi/pom.xml | Added tracing API dependency. |
| kar/src/main/feature/feature.xml | Added tracing bundles + web-servlets bundle + cfg provisioning to Karaf features. |
| itests/pom.xml | Updated dependencyManagement (formatting changed) related to tracing API. |
| extensions/router/router-core/pom.xml | Updated dependencyManagement (formatting changed) related to tracing impl. |
| extensions/json-schema/services/src/main/resources/OSGI-INF/blueprint/blueprint.xml | Injected TracerService into schema services bundle. |
| extensions/json-schema/services/src/main/java/org/apache/unomi/schema/impl/SchemaServiceImpl.java | Added validation trace info emission when tracing enabled. |
| extensions/json-schema/services/pom.xml | Added tracing API dependency (and impl for tests). |
| extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java | Added tracing around Groovy action execution. |
| extensions/groovy-actions/services/pom.xml | Added tracing API dependency (and impl for tests). |
| extensions/geonames/services/pom.xml | Updated dependencyManagement (formatting changed) related to tracing impl. |
| bom/artifacts/pom.xml | Added tracing + web-servlets artifacts to BOM. |
| api/src/main/java/org/apache/unomi/api/ContextResponse.java | Added requestTracing field to API response model. |
| api/pom.xml | Added tracing API dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…b-servlets for unomicfg
- Fix action executors (CopyProperties, EvaluateVisitProperties, EventToProfileProperty, GroovyActionDispatcher) unconditionally enabling tracing regardless of explain flag; now guard with isTracingEnabled() and null-check tracer before each call - Convert TraceNode context/result fields from Object to String, applying safeContextToString() in startOperation/endOperation to prevent Jackson serialization of arbitrary domain objects - Add cleanup() to TracerService interface; call it unconditionally in both endpoint finally blocks to prevent ThreadLocal leaks in Karaf thread pools - Fix endOperation/setRequestTracing ordering in both endpoints so root node endTime is set before the trace tree is captured - Fix inverted validation result booleans in RulesServiceImpl, SegmentServiceImpl and GoalsServiceImpl (isEmpty not !isEmpty) - Wrap validate() calls in try/catch in all three service impls to ensure endOperation is always called even if validation throws - Add null/enabled guards to SegmentServiceImpl hot-path tracer calls (isProfileInSegment, getSegmentsAndScoresForProfile, processScoring) to prevent NPE when tracerService is unbound - Add MAX_TRACE_DEPTH=100 guard in DefaultRequestTracer to bound trace tree size for complex rule sets - Return unmodifiable views from TraceNode.getChildren/getTraces; add addChild/addTrace mutators; clamp getDuration() to >= 0 - Remove redundant null check after String.valueOf in safeContextToString - Add tests for endOperation-without-startOperation and extra-endOperation-after-balanced-pair behaviours
…ssues - DefaultRequestTracer: add droppedOperations counter so endOperation() is a no-op for depth-limited spans instead of corrupting the node stack; add removeThreadLocals() to .remove() all 5 ThreadLocals on cleanup - DefaultTracerService.cleanup(): call removeThreadLocals() before removing the outer ThreadLocal to prevent OSGi classloader leaks - LoggingRequestTracer: override removeThreadLocals() to also remove indentLevel ThreadLocal - ContextJsonEndpoint / EventsCollectorEndpoint: move enableTracing() + startOperation() inside the try block (C1); guard disableTracing() and cleanup() in nested try/finally so neither can mask the original exception (H3); add LOGGER to EventsCollectorEndpoint - DefinitionsServiceImpl.resolveConditionType(): wrap validate() in try/catch so the open trace span is always closed on exception (H2) - SegmentServiceImpl: store getCurrentTracer() in local variable and null-check at all 4 call sites (M1) - GroovyActionDispatcher: make TracerService @reference optional/dynamic to match the null-guard in execute() (M2); add unsetTracerService() - HttpServletRequestForwardWrapper: null-check getContext("/cxs") with clear 503 error (L2); guard sendError() with isCommitted() check and its own try/catch so secondary IOException is not swallowed (H4)
…sues - DefaultTracerService: make getCurrentTracer() lazily allocate so non-explain requests never create a DefaultRequestTracer; make isTracingEnabled(), getTraceNode() and disableTracing() read currentTracer.get() directly to avoid triggering allocation - ContextJsonEndpoint / EventsCollectorEndpoint: null-guard tracerService in finally blocks to avoid silent ThreadLocal leak if the OSGi service is unbound mid-request - ConditionContextHelper: add isTracingEnabled() guard before the three getCurrentTracer() calls on error paths, preventing tracer creation on background threads that have no matching cleanup() call
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tracing-apiandtracing-implmodules with OSGiTracerService/RequestTracerfor the Explain feature.explainquery param on context/event collector).wabinto newweb-servletsmodule and registers bundles in Karaf features.Master review fixes from PRs C/C2 were preserved (recursion log dedup, parent-condition fallback,
IdsConditionEvaluatormatch semantics, etc.) rather than blind 3-dev overwrites.Test plan
mvn -pl tracing/tracing-impl test(DefaultTracerServiceTest)mvn -pl tracing,web-servlets,services,plugins/baseplugin,rest,kar -am install -DskipTests?explain=trueon/cxs/context.jsonor event collector as admin → trace tree in response