wenjin272 commented on code in PR #1219:
URL: https://github.com/apache/flink-agents/pull/1219#discussion_r4245179664
##########
api/src/main/java/org/apache/flink/agents/api/configuration/AgentConfigOptions.java:
##########
@@ -152,40 +149,56 @@ public enum ConditionEvaluationFailureStrategy {
new ConfigOption<>("job-identifier", String.class, null);
/**
- * The global event log level controlling the default verbosity for all
event types. Valid
- * values are "OFF", "STANDARD", and "VERBOSE". Defaults to "STANDARD".
+ * Trace log recording targets. Each target requires {@code scope}: either
{@code EVENT_ONLY},
+ * {@code ALL}, or an object with required {@code entityType} and optional
{@code entityName}.
+ * Names match exactly, except a suffix {@code .*}: {@code com.foo.*}
matches both {@code
+ * com.foo} and names beginning with {@code com.foo.}. A target's optional
{@code detail} is
+ * {@code OFF}, {@code STANDARD}, or {@code VERBOSE}. {@code OFF}
suppresses matching records;
+ * the other details affect attributes, while identities, relationships,
timestamps, and
+ * statuses remain complete.
+ *
+ * <p>Only matching entities whose effective detail is not {@code OFF} are
recorded. More
+ * specific scopes determine detail: exact name, longest name prefix,
entity type, {@code
+ * EVENT_ONLY}, then {@code ALL}. Target order does not matter. Defaults
to {@code EVENT_ONLY}
+ * with {@code STANDARD} detail; an explicitly empty list records no
entities. A more specific
+ * target can enable or disable recording even when a preset has the
opposite setting.
+ *
+ * <p>An omitted detail inherits from a preset, not another entity target.
Event targets prefer
+ * {@code EVENT_ONLY} over {@code ALL}; other entity targets use {@code
ALL}, falling back to
+ * {@code EVENT_ONLY} when it is the only preset. Without presets, entity
targets use {@code
+ * STANDARD}. An {@code ALL} target with omitted detail uses {@code
STANDARD}; an {@code
+ * EVENT_ONLY} target with omitted detail inherits {@code ALL}, or uses
{@code STANDARD} if
+ * {@code ALL} is absent.
*/
- public static final ConfigOption<EventLogLevel> EVENT_LOG_LEVEL =
- new ConfigOption<>("event-log.level", EventLogLevel.class,
EventLogLevel.STANDARD);
-
- /**
- * Whether to persist Agent Trace context and execution lifecycle Events
in the Event Log.
- * Business Events continue to be logged without trace context when this
option is disabled.
- * In-process execution reporting, including built-in metrics, is
unaffected.
- */
- public static final ConfigOption<Boolean> EVENT_LOG_TRACE_ENABLED =
- new ConfigOption<>("event-log.trace.enabled", Boolean.class,
false);
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ public static final ConfigOption<List<Map<String, Object>>>
TRACE_LOG_TARGETS =
Review Comment:
Could we make the configuration easier to use while keeping
`TraceLogTarget`, `TRACE_LOG_TARGETS`, and `trace-log.targets`?
I suggest:
- Rename `detail` to `level`, since `OFF / STANDARD / VERBOSE` controls both
whether a record is written and how much content is retained.
- Provide typed `TraceLogTarget` APIs in Java and Python, so users don't
need to construct nested maps or spell configuration keys.
- In YAML, support selectors such as `scope: {event: OrderCreated}` instead
of requiring `entityType / entityName`. Each scope object should contain
exactly one category selector.
For example, the proposed Java API could configure an existing environment
as follows:
```java
agentsEnv.getConfig().set(
AgentConfigOptions.TRACE_LOG_TARGETS,
List.of(
TraceLogTarget.events(TraceLogLevel.STANDARD),
TraceLogTarget.event(
"OrderCreated", TraceLogLevel.VERBOSE),
TraceLogTarget.action(
"create_order", TraceLogLevel.VERBOSE),
TraceLogTarget.tool(
"search_orders", TraceLogLevel.OFF)));
```
The equivalent Python API:
```python
agents_env.get_config().set(
AgentConfigOptions.TRACE_LOG_TARGETS,
[
TraceLogTarget.events(TraceLogLevel.STANDARD),
TraceLogTarget.event("OrderCreated", TraceLogLevel.VERBOSE),
TraceLogTarget.action("create_order", TraceLogLevel.VERBOSE),
TraceLogTarget.tool("search_orders", TraceLogLevel.OFF),
],
)
```
And the equivalent YAML:
```yaml
agent:
trace-log:
targets:
- scope: EVENT_ONLY
level: STANDARD
- scope:
event: OrderCreated
level: VERBOSE
- scope:
action: create_order
level: VERBOSE
- scope:
tool: search_orders
level: "OFF"
```
These forms should share the same validation and resolution logic. The
`detail` → `level` rename should also be consistent across the enum,
configuration, and output JSON. For custom execution categories supported by
`ExecutionReporter`, we should retain a generic selector alongside the
convenience APIs.
##########
runtime/src/main/java/org/apache/flink/agents/runtime/tracelog/TraceLogTargetResolver.java:
##########
@@ -0,0 +1,323 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.agents.runtime.tracelog;
+
+import org.apache.flink.agents.api.configuration.AgentConfigOptions;
+import org.apache.flink.agents.api.logger.TraceLogDetail;
+import org.apache.flink.agents.api.logger.TraceLogScope;
+import org.apache.flink.agents.api.trace.TraceContext;
+import org.apache.flink.agents.api.trace.TraceRecord;
+
+import javax.annotation.Nullable;
+
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/** Selects records and resolves their recording detail from trace log
targets. */
+final class TraceLogTargetResolver {
+ private static final Set<String> TARGET_FIELDS = Set.of("scope", "detail");
+ private static final Set<String> SCOPE_FIELDS = Set.of("entityType",
"entityName");
+ private static final Set<String> LEGACY_OUTPUT_KEYS =
+ Set.of("eventLoggerType", "baseLogDir", "prettyPrint");
+
+ @Nullable private final TraceLogDetail allDetail;
+ @Nullable private final TraceLogDetail eventDetail;
+ private final Map<String, TypeTargets> typeTargets = new LinkedHashMap<>();
+ private final List<Target> targets = new ArrayList<>();
+
+ /** Reads and validates targets before logging starts. */
+ TraceLogTargetResolver(@Nullable Map<String, Object> confData) {
+ Map<String, Object> data = confData == null ? Collections.emptyMap() :
confData;
+ rejectLegacySettings(data);
+ String targetsKey = AgentConfigOptions.TRACE_LOG_TARGETS.getKey();
+ if (data.keySet().stream().anyMatch(key -> key.startsWith(targetsKey +
"."))) {
+ throw invalidTargets("must be a list of targets, not a nested
object");
+ }
+ Object configuredTargets =
+ data.getOrDefault(
+ targetsKey,
AgentConfigOptions.TRACE_LOG_TARGETS.getDefaultValue());
+ if (!(configuredTargets instanceof List)) {
+ throw invalidTargets("must be a list of targets");
+ }
+ List<Target> configured = new ArrayList<>();
+ int index = 0;
+ for (Object configuredTarget : (List<?>) configuredTargets) {
+ configured.add(parseTarget(configuredTarget, index++));
+ }
+ allDetail = resolvePresetDetail(configured, TraceLogScope.ALL,
TraceLogDetail.STANDARD);
+ TraceLogDetail eventOnlyDetail =
+ resolvePresetDetail(
+ configured,
+ TraceLogScope.EVENT_ONLY,
+ allDetail == null ? TraceLogDetail.STANDARD :
allDetail);
+ eventDetail = eventOnlyDetail == null ? allDetail : eventOnlyDetail;
+ TraceLogDetail inheritedDetail =
+ allDetail != null
+ ? allDetail
+ : eventOnlyDetail != null ? eventOnlyDetail :
TraceLogDetail.STANDARD;
+
+ Map<Object, TraceLogDetail> selectors = new LinkedHashMap<>();
+ for (Target target : configured) {
+ TraceLogDetail presetDefault = presetDetail(target.entityType);
+ TraceLogDetail detail =
+ target.detailOr(presetDefault == null ? inheritedDetail :
presetDefault);
Review Comment:
Could we keep the recording level optional, but inherit only from broader
targets that also match the record?
Currently, the following configuration disables `search` Tool logging as
well as Event logging, because the Tool inherits `OFF` from `EVENT_ONLY` (using
the current field names):
```yaml
agent:
trace-log:
targets:
- scope: EVENT_ONLY
detail: "OFF"
- scope:
entityType: tool
entityName: search
```
I would expect `search` to use STANDARD here: an Event-only setting should
not affect Tools.
Similarly, if all Tools are configured as VERBOSE, adding a specific Tool
target without a level currently makes that Tool use STANDARD instead of
inheriting the Tool-wide setting.
Could we resolve levels as follows?
- Among targets matching the record, use the most specific target that
explicitly specifies a level, following the existing specificity order.
- If targets match but none specifies a level, use STANDARD.
- If no target matches, omit the record.
For example, a Tool target without a level would inherit VERBOSE from an
`ALL: VERBOSE` or Tool-wide VERBOSE target. An explicit level on that Tool
would override the broader setting.
This preserves optional levels while making inheritance follow the same
matching hierarchy, without the special cross-category inheritance from
`EVENT_ONLY` to Tools.
--
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]
