fix: make pod_name and container_name optional in LoggingContext; add ContainerIdentifier to TailLogsResponse#7341
Open
pingsutw wants to merge 21 commits into
Open
fix: make pod_name and container_name optional in LoggingContext; add ContainerIdentifier to TailLogsResponse#7341pingsutw wants to merge 21 commits into
pingsutw wants to merge 21 commits into
Conversation
…name fields Signed-off-by: Kevin Su <pingsutw@apache.org>
… container_name Signed-off-by: Kevin Su <pingsutw@apache.org>
Add a container field to TailLogsResponse.Logs so callers can identify which pod/container each batch of log lines came from, enabling per-pod filtering in the log viewer UI. Signed-off-by: Kevin Su <pingsutw@apache.org>
…ging-context-optional-pod-fields
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Flyte IDL log-related protos to support label-based log queries (by relaxing validation on LoggingContext pod/container name fields) and to let log-tail clients attribute streamed log batches to a specific pod/container (by adding a ContainerIdentifier to TailLogsResponse.Logs). Generated Go/Python/TypeScript bindings are regenerated to reflect these schema changes.
Changes:
- Remove buf
string.min_len = 1validation fromLoggingContext.kubernetes_pod_nameandLoggingContext.kubernetes_container_name. - Add
flyteidl2.logs.dataplane.ContainerIdentifier container = 2toflyteidl2.dataproxy.TailLogsResponse.Logs. - Regenerate TS/Python/Go protobuf outputs (including Go validation stubs).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| gen/ts/flyteidl2/logs/dataplane/payload_pb.ts | Regenerated TS descriptor for updated LoggingContext validation rules. |
| gen/ts/flyteidl2/dataproxy/dataproxy_service_pb.ts | Regenerated TS types to include TailLogsResponse.Logs.container. |
| gen/python/flyteidl2/logs/dataplane/payload_pb2.py | Regenerated Python proto to remove validation options on LoggingContext pod/container names. |
| gen/python/flyteidl2/dataproxy/dataproxy_service_pb2.pyi | Regenerated Python typing stub to include TailLogsResponse.Logs.container. |
| gen/python/flyteidl2/dataproxy/dataproxy_service_pb2.py | Regenerated Python proto to include TailLogsResponse.Logs.container. |
| gen/go/flyteidl2/logs/dataplane/payload.pb.go | Regenerated Go proto for updated LoggingContext schema. |
| gen/go/flyteidl2/dataproxy/dataproxy_service.pb.validate.go | Regenerated Go validation code to validate TailLogsResponse.Logs.container when present. |
| gen/go/flyteidl2/dataproxy/dataproxy_service.pb.go | Regenerated Go proto to include TailLogsResponse.Logs.container. |
| flyteidl2/logs/dataplane/payload.proto | Drops min_len validation on LoggingContext pod/container name fields. |
| flyteidl2/dataproxy/dataproxy_service.proto | Adds ContainerIdentifier container to TailLogsResponse.Logs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…name) Replace string pod_name with oneof pod_selector. Default (unset) and all_pods stream all pods; primary_pod streams only the primary pod; pod_name streams a specific named pod. Signed-off-by: Kevin Su <pingsutw@apache.org>
Enables callers to scope persisted log queries to the specific action attempt without relying on container-level process timestamps. Signed-off-by: Kevin Su <pingsutw@apache.org>
…es_container_name Signed-off-by: Kevin Su <pingsutw@apache.org>
katrogan
previously approved these changes
May 8, 2026
| // The pod name to tail logs from. If not provided, attempt to find the primary pod, else assume the first pod. | ||
| string pod_name = 3; | ||
| // Selects which pod(s) to tail. If unset, defaults to primary_pod. | ||
| oneof pod_selector { |
Contributor
There was a problem hiding this comment.
nit: maybe we can call this pod_filter and then we can drop the all_pods option altogether since we default to it when no filter is set
| oneof pod_selector { | ||
| // Tail logs from the primary pod only. | ||
| bool primary_pod = 6; | ||
| // Tail logs from all pods. This is the default when pod_selector is unset. |
Contributor
There was a problem hiding this comment.
this is a departure from the currently checked in behavior - will it cause any issues for older UI clients?
Member
Author
There was a problem hiding this comment.
sorry for the confusion. The default is primary pod (same as before). I just updated the comment
Add a dedicated uint32 execution_attempt field (field 12) to LoggingContext so log sources can compute pod uniqueIDs that match flytepropeller's FixedLengthUniqueIDForParts hashing without polluting kubernetes_pod_labels with non-k8s metadata. Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
katrogan
previously approved these changes
May 12, 2026
| oneof pod_selector { | ||
| // Tail logs from the primary pod only. | ||
| oneof pod_filter { | ||
| // Tail logs from the primary pod only. This is the default when pod_filter is unset. |
Contributor
There was a problem hiding this comment.
oh I see, if this is the default totally fine keeping the oneof field name pod_selector then
…ging-context-optional-pod-fields
…ptional-pod-fields Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
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.
Why
LoggingContext.kubernetes_pod_nameandkubernetes_container_namehad amin_len = 1buf validation constraint, which rejected label-based log requests where the caller provides pod labels instead of a specific pod name. Union Cloud's DP dataproxy uses label-based queries for multi-pod jobs (e.g. Ray) where the specific pod name is not known upfront.TailTaskExecutionLogsResponse.Logswas also missing acontainerfield needed by the client to attribute log lines to specific pod/container pairs in the UI dropdown.What
min_lenfromLoggingContext.kubernetes_pod_nameandkubernetes_container_namefieldsContainerIdentifier container = 3toTailTaskExecutionLogsResponse.LogsTesting
Verified via Union Cloud DP dataproxy integration — label-based log queries now pass proto validation and the container identifier is correctly surfaced to the client.