wmedvede commented on code in PR #4094:
URL:
https://github.com/apache/incubator-kie-kogito-runtimes/pull/4094#discussion_r2585264363
##########
jbpm/jbpm-flow/src/main/java/org/kie/kogito/process/impl/AbstractProcessInstance.java:
##########
@@ -225,9 +226,35 @@ protected void reconnect() {
}
getProcessRuntime().getProcessInstanceManager().setLock(((MutableProcessInstances<T>)
process.instances()).lock());
processInstance.setMetaData(KOGITO_PROCESS_INSTANCE, this);
+
+ restoreProcessInstanceContext();
+
processInstance.reconnect();
}
+ private void restoreProcessInstanceContext() {
+ Map<String, List<String>> headers = processInstance.getHeaders();
+ if (headers != null && !headers.isEmpty()) {
+ Map<String, String> contextMap =
convertHeadersToContextMap(headers);
+ if (!contextMap.isEmpty()) {
+
org.kie.kogito.services.context.ProcessInstanceContext.setContextFromAsync(contextMap);
Review Comment:
This is good, so we restore the ProcessInstance, and then, we "restore" the
context in the MDC by doing
.ProcessInstanceContext.setContextFromAsync(contextMap).
Then, internally, the ProcessInstanceContext will do:
MDC.setContextMap(theNewValues);
:point_up: and now the MDC context map will have the new values. (cleaning
the existing ones)
But considering that the invocation to the restoreProcessInstanceContext()
was produced by a caller thread, that is not under our control, aren't where
here making the caller thread to loose any potential values stored by him in
the MDC?
Does it make sense?
Maybe es need a sort of Merge. Thinking aloud.
##########
quarkus/addons/token-exchange/runtime/src/main/java/org/kie/kogito/addons/quarkus/token/exchange/cache/TokenEvictionHandler.java:
##########
@@ -96,28 +116,41 @@ private void onTokenExpired(String cacheKey, CachedTokens
tokens, RemovalCause c
/**
* Refreshes tokens using a cached refresh token and updates the cache.
- *
+ * The process instance context is set based on the cache key.
+ *
* @param cacheKey The cache key for the tokens being refreshed
* @param refreshToken The refresh token to use for getting new tokens
*/
private void refreshWithCachedToken(String cacheKey, String refreshToken) {
+ // Extract process instance ID from cache key for context propagation
+ String processInstanceId =
CacheUtils.extractProcessInstanceIdFromCacheKey(cacheKey);
+
tokenRefreshExecutor.submit(() -> {
try {
+ ProcessInstanceContext.setProcessInstanceId(processInstanceId);
Review Comment:
related to the comment above, we don't mind, we just set
##########
quarkus/addons/token-exchange/runtime/src/main/java/org/kie/kogito/addons/quarkus/token/exchange/cache/TokenEvictionHandler.java:
##########
@@ -54,29 +56,47 @@ public TokenEvictionHandler(TokenCRUD tokenCRUD) {
/**
* Creates a removal listener that can be used with Caffeine cache.
- *
+ * This listener sets the proper process instance context from the cache
key before logging.
+ *
* @return A removal listener that handles token eviction events
*/
public RemovalListener<String, CachedTokens> createRemovalListener() {
return (key, value, cause) -> {
if (value == null)
return;
- LOGGER.info("Token cache eviction for cache key '{}' - Cause: {}",
key, cause);
- onTokenExpired(key, value, cause);
+ // Extract process instance ID from cache key and set context for
logging
+ String processInstanceId =
CacheUtils.extractProcessInstanceIdFromCacheKey(key);
+ if (processInstanceId != null && !processInstanceId.isEmpty()) {
+ ProcessInstanceContext.setProcessInstanceId(processInstanceId);
Review Comment:
No Id, we don't set.
##########
addons/common/monitoring/core/src/main/java/org/kie/kogito/monitoring/core/common/process/MetricsProcessEventListener.java:
##########
@@ -134,10 +141,37 @@ protected static double millisToSeconds(long millis) {
return millis / 1000.0;
}
+ /**
+ * Gets the current process instance ID from MDC for correlation.
+ *
+ * @return the process instance ID, or null if not available
+ */
+ private String getProcessInstanceIdFromContext() {
+ return MDC.get(MDC_PROCESS_INSTANCE_KEY);
+ }
+
+ /**
+ * Logs metric correlation information for debugging and observability.
+ *
+ * @param metricName the name of the metric being recorded
+ * @param processId the process ID
+ * @param processInstanceId the process instance ID (may be null)
+ */
+ private void logMetricCorrelation(String metricName, String processId,
String processInstanceId) {
+ if (LOGGER.isDebugEnabled() && processInstanceId != null) {
+ LOGGER.debug("Metric {} for process {} instance {}",
+ metricName, processId, processInstanceId);
+ }
+ }
+
@Override
public void afterProcessStarted(ProcessStartedEvent event) {
LOGGER.debug("After process started event: {}", event);
final ProcessInstance processInstance = event.getProcessInstance();
+ String processInstanceId = getProcessInstanceIdFromContext();
+
+ logMetricCorrelation("process_started",
processInstance.getProcessId(), processInstanceId);
Review Comment:
maybe just logMetricCorrelation("process_started",
processInstance.getProcessId(), getProcessInstanceIdFromContext()); if we
preserve this log line, and avoid the variable.
Same for the other invocations.
##########
quarkus/addons/token-exchange/runtime/src/main/java/org/kie/kogito/addons/quarkus/token/exchange/cache/TokenEvictionHandler.java:
##########
@@ -54,29 +56,47 @@ public TokenEvictionHandler(TokenCRUD tokenCRUD) {
/**
* Creates a removal listener that can be used with Caffeine cache.
- *
+ * This listener sets the proper process instance context from the cache
key before logging.
+ *
* @return A removal listener that handles token eviction events
*/
public RemovalListener<String, CachedTokens> createRemovalListener() {
return (key, value, cause) -> {
if (value == null)
return;
- LOGGER.info("Token cache eviction for cache key '{}' - Cause: {}",
key, cause);
- onTokenExpired(key, value, cause);
+ // Extract process instance ID from cache key and set context for
logging
+ String processInstanceId =
CacheUtils.extractProcessInstanceIdFromCacheKey(key);
+ if (processInstanceId != null && !processInstanceId.isEmpty()) {
+ ProcessInstanceContext.setProcessInstanceId(processInstanceId);
+ }
+
+ try {
+ LOGGER.info("Token cache eviction for cache key '{}' - Cause:
{}", key, cause);
+ onTokenExpired(key, value, cause);
+ } finally {
+ // Reset to general context after logging
+ ProcessInstanceContext.clear();
+ }
};
}
/**
* Callback method called when tokens are evicted from the cache.
- *
+ * Context is already set by the removal listener, so no need to set it
here.
+ *
* @param cacheKey The cache key of the evicted tokens
* @param tokens The evicted token data
* @param cause The reason for eviction (Caffeine's RemovalCause)
*/
private void onTokenExpired(String cacheKey, CachedTokens tokens,
RemovalCause cause) {
LOGGER.warn("OAuth2 tokens for cache key '{}' have expired/been
evicted: {}", cacheKey, cause);
+ String processInstanceId =
CacheUtils.extractProcessInstanceIdFromCacheKey(cacheKey);
Review Comment:
Wasn't already set by the caller method?
Maybe we don't need to set again.
##########
quarkus/addons/token-exchange/runtime/src/main/java/org/kie/kogito/addons/quarkus/token/exchange/cache/TokenEvictionHandler.java:
##########
@@ -54,29 +56,47 @@ public TokenEvictionHandler(TokenCRUD tokenCRUD) {
/**
* Creates a removal listener that can be used with Caffeine cache.
- *
+ * This listener sets the proper process instance context from the cache
key before logging.
+ *
* @return A removal listener that handles token eviction events
*/
public RemovalListener<String, CachedTokens> createRemovalListener() {
return (key, value, cause) -> {
if (value == null)
return;
- LOGGER.info("Token cache eviction for cache key '{}' - Cause: {}",
key, cause);
- onTokenExpired(key, value, cause);
+ // Extract process instance ID from cache key and set context for
logging
+ String processInstanceId =
CacheUtils.extractProcessInstanceIdFromCacheKey(key);
+ if (processInstanceId != null && !processInstanceId.isEmpty()) {
+ ProcessInstanceContext.setProcessInstanceId(processInstanceId);
+ }
+
+ try {
+ LOGGER.info("Token cache eviction for cache key '{}' - Cause:
{}", key, cause);
+ onTokenExpired(key, value, cause);
+ } finally {
+ // Reset to general context after logging
+ ProcessInstanceContext.clear();
Review Comment:
But we always clear. Not sure if it looks consistent with the
```
if (processInstanceId != null && !processInstanceId.isEmpty()) {
ProcessInstanceContext.setProcessInstanceId(processInstanceId);
```
Maybe we can always do
ProcessInstanceContext.setProcessInstanceId(processInstanceId);, internally I
think the nul case is already managed.
Does it make sense? Thinking aloud
##########
quarkus/addons/token-exchange/runtime/src/main/java/org/kie/kogito/addons/quarkus/token/exchange/cache/TokenPolicyManager.java:
##########
@@ -55,25 +56,37 @@ public long expireAfterRead(String key, CachedTokens value,
long currentTime, lo
}
/**
- * Calculate time to expiration based on token's actual expiration time
minus proactive refresh buffer
+ * Calculate time to expiration based on token's actual expiration time
minus proactive refresh buffer.
+ * This method sets the process instance context from the cache key for
proper logging.
*/
private static long calculateTimeToExpiration(String cacheKey,
CachedTokens tokens) {
- String authName = CacheUtils.extractAuthNameFromCacheKey(cacheKey);
- long proactiveRefreshSeconds = Math.max(0,
ConfigReaderUtils.getProactiveRefreshSeconds(authName));
+ // Extract process instance ID from cache key and set context for
logging
+ String processInstanceId =
CacheUtils.extractProcessInstanceIdFromCacheKey(cacheKey);
+ if (processInstanceId != null && !processInstanceId.isEmpty()) {
+ ProcessInstanceContext.setProcessInstanceId(processInstanceId);
Review Comment:
same comment here, we just set by no asking.
does it make sense?
##########
addons/common/monitoring/core/src/main/java/org/kie/kogito/monitoring/core/common/process/MetricsProcessEventListener.java:
##########
@@ -134,10 +141,37 @@ protected static double millisToSeconds(long millis) {
return millis / 1000.0;
}
+ /**
+ * Gets the current process instance ID from MDC for correlation.
+ *
+ * @return the process instance ID, or null if not available
+ */
+ private String getProcessInstanceIdFromContext() {
+ return MDC.get(MDC_PROCESS_INSTANCE_KEY);
+ }
+
+ /**
+ * Logs metric correlation information for debugging and observability.
+ *
+ * @param metricName the name of the metric being recorded
+ * @param processId the process ID
+ * @param processInstanceId the process instance ID (may be null)
+ */
+ private void logMetricCorrelation(String metricName, String processId,
String processInstanceId) {
+ if (LOGGER.isDebugEnabled() && processInstanceId != null) {
Review Comment:
What are these logs desined for?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
