wenjin272 commented on code in PR #609: URL: https://github.com/apache/flink-agents/pull/609#discussion_r3061799790
########## runtime/src/main/java/org/apache/flink/agents/runtime/eventlog/EventLogLevelResolver.java: ########## @@ -0,0 +1,145 @@ +/* + * 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.eventlog; + +import org.apache.flink.agents.api.logger.EventLogLevel; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Resolves the effective {@link EventLogLevel} for a given event type string using hierarchical + * config key inheritance. + * + * <p>Resolution order for a given event type (e.g., {@code org.apache.flink.agents.api.event + * .ChatRequestEvent}): + * + * <ol> + * <li>Exact match in explicit per-type configuration + * <li>Walk up dot-separated segments (e.g., {@code org.apache.flink.agents.api.event}, then + * {@code org.apache.flink.agents.api}, etc.) + * <li>Root default from {@code event-log.level} config key + * <li>Built-in default: {@link EventLogLevel#STANDARD} + * </ol> + * + * <p>This mirrors Log4j's hierarchical logger configuration pattern. Resolved levels are cached in + * a {@link ConcurrentHashMap} for efficient repeated lookups. + * + * <p>Config keys are expected in the form: + * + * <pre> + * event-log.level = STANDARD (root default) + * event-log.type.<EVENT_TYPE>.level = OFF (per-type override) + * </pre> + */ +public class EventLogLevelResolver { + + /** Config key for the root default log level. */ + static final String ROOT_LEVEL_KEY = "event-log.level"; + + /** Prefix for per-event-type log level config keys. */ + static final String TYPE_PREFIX = "event-log.type."; + + /** Suffix for per-event-type log level config keys. */ + static final String TYPE_SUFFIX = ".level"; + + /** Built-in default when no configuration is provided. */ + static final EventLogLevel BUILT_IN_DEFAULT = EventLogLevel.STANDARD; + + private final EventLogLevel rootDefault; + private final Map<String, EventLogLevel> explicitLevels; + private final ConcurrentHashMap<String, EventLogLevel> cache; + + /** + * Creates a resolver from a configuration data map. + * + * <p>The map is scanned for keys matching {@code event-log.type.<EVENT_TYPE>.level} to build + * the explicit per-type level mappings, and {@code event-log.level} for the root default. + * + * @param confData the flat configuration key-value map (e.g., from {@code + * AgentConfiguration.getConfData()}) + */ + public EventLogLevelResolver(Map<String, Object> confData) { + Map<String, Object> data = confData != null ? confData : Collections.emptyMap(); + + // Parse root default + Object rootValue = data.get(ROOT_LEVEL_KEY); Review Comment: Shall we just use `EVENT_LOG_LEVEL.getKey()` here? ########## runtime/src/main/java/org/apache/flink/agents/runtime/eventlog/EventLogLevelResolver.java: ########## @@ -0,0 +1,145 @@ +/* + * 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.eventlog; + +import org.apache.flink.agents.api.logger.EventLogLevel; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Resolves the effective {@link EventLogLevel} for a given event type string using hierarchical + * config key inheritance. + * + * <p>Resolution order for a given event type (e.g., {@code org.apache.flink.agents.api.event + * .ChatRequestEvent}): + * + * <ol> + * <li>Exact match in explicit per-type configuration + * <li>Walk up dot-separated segments (e.g., {@code org.apache.flink.agents.api.event}, then + * {@code org.apache.flink.agents.api}, etc.) + * <li>Root default from {@code event-log.level} config key + * <li>Built-in default: {@link EventLogLevel#STANDARD} + * </ol> + * + * <p>This mirrors Log4j's hierarchical logger configuration pattern. Resolved levels are cached in + * a {@link ConcurrentHashMap} for efficient repeated lookups. + * + * <p>Config keys are expected in the form: + * + * <pre> + * event-log.level = STANDARD (root default) + * event-log.type.<EVENT_TYPE>.level = OFF (per-type override) + * </pre> + */ +public class EventLogLevelResolver { + + /** Config key for the root default log level. */ + static final String ROOT_LEVEL_KEY = "event-log.level"; + + /** Prefix for per-event-type log level config keys. */ + static final String TYPE_PREFIX = "event-log.type."; + + /** Suffix for per-event-type log level config keys. */ + static final String TYPE_SUFFIX = ".level"; + + /** Built-in default when no configuration is provided. */ + static final EventLogLevel BUILT_IN_DEFAULT = EventLogLevel.STANDARD; + + private final EventLogLevel rootDefault; + private final Map<String, EventLogLevel> explicitLevels; + private final ConcurrentHashMap<String, EventLogLevel> cache; + + /** + * Creates a resolver from a configuration data map. + * + * <p>The map is scanned for keys matching {@code event-log.type.<EVENT_TYPE>.level} to build + * the explicit per-type level mappings, and {@code event-log.level} for the root default. + * + * @param confData the flat configuration key-value map (e.g., from {@code + * AgentConfiguration.getConfData()}) + */ + public EventLogLevelResolver(Map<String, Object> confData) { + Map<String, Object> data = confData != null ? confData : Collections.emptyMap(); + + // Parse root default + Object rootValue = data.get(ROOT_LEVEL_KEY); + this.rootDefault = + rootValue != null + ? EventLogLevel.fromString(rootValue.toString()) + : BUILT_IN_DEFAULT; Review Comment: Shall we just use `EVENT_LOG_LEVEL.getDefaultValue()` here? ########## runtime/src/main/java/org/apache/flink/agents/runtime/eventlog/EventLogRecordJsonSerializer.java: ########## @@ -67,15 +70,37 @@ public void serialize(EventLogRecord record, JsonGenerator gen, SerializerProvid gen.writeStartObject(); gen.writeStringField("timestamp", record.getContext().getTimestamp()); + gen.writeStringField( + "logLevel", record.getLogLevel() != null ? record.getLogLevel().name() : "VERBOSE"); + // For PythonEvent, use the Python module path as the top-level eventType + // (consistent with the eventType inside the event object) + String topLevelEventType = record.getContext().getEventType(); + if (record.getEvent() instanceof PythonEvent) { + String pythonType = ((PythonEvent) record.getEvent()).getEventType(); + if (pythonType != null) { + topLevelEventType = pythonType; + } + } Review Comment: The logic for extracting the python event type is the same as in `FileEventLogger`. We can extract a common util function. ########## api/src/main/java/org/apache/flink/agents/api/configuration/AgentConfigOptions.java: ########## @@ -54,4 +54,32 @@ public class AgentConfigOptions { /** The config parameter specifies the unique identifier of job. */ public static final ConfigOption<String> JOB_IDENTIFIER = 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". + */ + public static final ConfigOption<String> EVENT_LOG_LEVEL = + new ConfigOption<>("event-log.level", String.class, "STANDARD"); Review Comment: Maybe we can just use enum here ``` public static final ConfigOption<EventLogLevel> EVENT_LOG_LEVEL = new ConfigOption<>("event-log.level", EventLogLevel.class, EventLogLevel.STANDARD); ``` ########## runtime/src/main/java/org/apache/flink/agents/runtime/operator/ActionExecutionOperator.java: ########## @@ -342,6 +342,10 @@ private void initEventLogger(StreamingRuntimeContext runtimeContext) throws Exce return; } eventLogger.open(new EventLoggerOpenParams(runtimeContext)); + if (eventLogger instanceof FileEventLogger) { Review Comment: Why is it necessary to specifically check the type of `eventLogger` here? ########## runtime/src/main/java/org/apache/flink/agents/runtime/eventlog/EventLogRecord.java: ########## @@ -31,23 +33,82 @@ * * <p>The class uses custom JSON serialization/deserialization to handle polymorphic Event types by * leveraging the eventType information stored in the EventContext. + * + * <p>Each record carries a {@link EventLogLevel} that controls truncation behavior during + * serialization, and a convenience copy of the event type string from the context. */ @JsonSerialize(using = EventLogRecordJsonSerializer.class) @JsonDeserialize(using = EventLogRecordJsonDeserializer.class) public class EventLogRecord { private final EventContext context; private final Event event; + private final EventLogLevel logLevel; + private final String eventType; + private final transient JsonTruncator truncator; + private final transient Counter truncatedEventsCounter; Review Comment: Maybe we can mark truncator and truncatedEventsCounter as `@Nullable`. Additionally, these two members are actually required variables for the serialization of `EventLogRecord`, rather than part of its content. During deserialization, we have to set them to `null`. I wonder if they could be removed from within `EventLogRecord` itself. -- 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]
