Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for generic dynamic log links #5066

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion flyteplugins/go/tasks/logs/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ type LogConfig struct {
StackdriverLogResourceName string `json:"stackdriver-logresourcename" pflag:",Name of the logresource in stackdriver"`
StackDriverTemplateURI tasklog.TemplateURI `json:"stackdriver-template-uri" pflag:",Template Uri to use when building stackdriver log links"`

DynamicLogLinks map[string]tasklog.TemplateLogPlugin `json:"dynamic-log-links" pflag:"-,Map of dynamic log links"`
DynamicLogLinks map[string]tasklog.TemplateLogPlugin `json:"dynamic-log-links" pflag:"-,Map of dynamic log links"`
GenericDynamicLogLinksEnabled bool `json:"generic-dynamic-log-links-enabled" pflag:",Enable generic dynamic log links"`

Templates []tasklog.TemplateLogPlugin `json:"templates" pflag:"-,"`
}
Expand Down
1 change: 1 addition & 0 deletions flyteplugins/go/tasks/logs/logconfig_flags.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions flyteplugins/go/tasks/logs/logconfig_flags_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions flyteplugins/go/tasks/logs/logging_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ func InitializeLogPlugins(cfg *LogConfig) (tasklog.Plugin, error) {
})
}

// The generic dynamic log link is special. We're going to use the string `Generic` to signal that we're using it.
// TODO: decide if it's worth doing this in a different way.
if cfg.GenericDynamicLogLinksEnabled {
dynamicPlugins = append(dynamicPlugins, tasklog.TemplateLogPlugin{Name: "Generic", DynamicTemplateURIs: []string{""}, MessageFormat: core.TaskLog_JSON})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type mismatch in DynamicTemplateURIs assignment

Consider using []tasklog.TemplateURI instead of []string for DynamicTemplateURIs to maintain type consistency. The TemplateLogPlugin struct expects DynamicTemplateURIs to be of type []TemplateURI.

Code suggestion
Check the AI-generated fix before applying
Suggested change
dynamicPlugins = append(dynamicPlugins, tasklog.TemplateLogPlugin{Name: "Generic", DynamicTemplateURIs: []string{""}, MessageFormat: core.TaskLog_JSON})
dynamicPlugins = append(dynamicPlugins, tasklog.TemplateLogPlugin{Name: "Generic", DynamicTemplateURIs: []tasklog.TemplateURI{""}, MessageFormat: core.TaskLog_JSON})

Code Review Run #51871a


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

}

plugins = append(plugins, cfg.Templates...)
return templateLogPluginCollection{plugins: plugins, dynamicPlugins: dynamicPlugins}, nil
}
201 changes: 201 additions & 0 deletions flyteplugins/go/tasks/logs/logging_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -566,3 +566,204 @@ func TestGetLogsForContainerInPod_Flyteinteractive(t *testing.T) {
})
}
}

func TestGetLogsForContainerInPod_GenericDynamicLogLinks(t *testing.T) {
tests := []struct {
name string
config *LogConfig
template *core.TaskTemplate
expectedTaskLogs []*core.TaskLog
}{
{
"Generic Dynamic Log Links enabled but no task template",
&LogConfig{
GenericDynamicLogLinksEnabled: true,
},
nil,
nil,
},
{
"Generic Dynamic Log Links enabled and empty task template",
&LogConfig{
GenericDynamicLogLinksEnabled: true,
},
&core.TaskTemplate{},
nil,
},
{
"Generic Dynamic Log Links enabled and simple config present in TaskTemplate",
&LogConfig{
GenericDynamicLogLinksEnabled: true,
},
&core.TaskTemplate{
Config: map[string]string{
"link_type": "vscode",
"port": "65535",
"generic_dynamic_log_links": `{"links": [{"name": "vscode", "display_name": "vscode", "template_uri": "vscode://def.com:{{ .taskConfig.port }}/{{ .podName }}"}]}`,
},
},
[]*core.TaskLog{
{
Uri: "vscode://def.com:65535/my-pod",
MessageFormat: core.TaskLog_JSON,
Name: "vscode",
},
},
},
{
"Generic Dynamic Log Links disabled and simple config present in TaskTemplate - Notice how we can mix and match parameters defined in the task template, i.e. using {{ .taskConfig.port }} also works",
&LogConfig{},
&core.TaskTemplate{
Config: map[string]string{
"link_type": "vscode",
"port": "65535",
"generic_dynamic_log_links": `{"links": [{"name": "vscode", "display_name": "vscode", "template_uri": "vscode://def.com:{{ .taskConfig.port }}/{{ .podName }}"}]}`,
},
},
nil,
},
{
"Generic Dynamic Log Links - multiple dynamic options",
&LogConfig{
GenericDynamicLogLinksEnabled: true,
DynamicLogLinks: map[string]tasklog.TemplateLogPlugin{
"vscode": tasklog.TemplateLogPlugin{
DisplayName: "vscode link",
TemplateURIs: []tasklog.TemplateURI{
"https://abc.com:{{ .taskConfig.port }}/{{ .taskConfig.route }}",
},
},
},
},
&core.TaskTemplate{
Config: map[string]string{
"link_type": "vscode",
"port": "65535",
"route": "a-route",
"generic_dynamic_log_links": `{"links": [{"name": "vscode", "display_name": "Other logs", "template_uri": "coming://from/other/place"}]}`,
},
},
[]*core.TaskLog{
{
Uri: "https://abc.com:65535/a-route",
MessageFormat: core.TaskLog_JSON,
Name: "vscode link my-Suffix",
},
{
Uri: "coming://from/other/place",
MessageFormat: core.TaskLog_JSON,
Name: "Other logs",
},
},
},
{
"Generic Dynamic Log Links disabled and K8s enabled and generic dynamic log links config present in TaskTemplate",
&LogConfig{
IsKubernetesEnabled: true,
KubernetesTemplateURI: "https://k8s.com/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}",
GenericDynamicLogLinksEnabled: false,
},
&core.TaskTemplate{
Config: map[string]string{
"link_type": "abc",
"generic_dynamic_log_links": `{"links": [{"display_name": "Other logs", "template_uri": "coming://from/other/place"}]}`,
},
},
[]*core.TaskLog{
{
Uri: "https://k8s.com/my-namespace/my-pod/ContainerName/ContainerID",
MessageFormat: core.TaskLog_JSON,
Name: "Kubernetes Logs my-Suffix",
},
},
},
{
"Generic Dynamic Log Links enabled and K8s enabled and generic dynamic log links config present in TaskTemplate",
&LogConfig{
IsKubernetesEnabled: true,
KubernetesTemplateURI: "https://k8s.com/{{ .namespace }}/{{ .podName }}/{{ .containerName }}/{{ .containerId }}",
GenericDynamicLogLinksEnabled: true,
},
&core.TaskTemplate{
Config: map[string]string{
"link_type": "abc",
"generic_dynamic_log_links": `{"links": [{"name": "abc", "display_name": "Other logs", "template_uri": "coming://from/other/place"}]}`,
},
},
[]*core.TaskLog{
{
Uri: "https://k8s.com/my-namespace/my-pod/ContainerName/ContainerID",
MessageFormat: core.TaskLog_JSON,
Name: "Kubernetes Logs my-Suffix",
},
{
Uri: "coming://from/other/place",
MessageFormat: core.TaskLog_JSON,
Name: "Other logs",
},
},
},
{
"Multiple Generic Dynamic Log Links defined in task template",
&LogConfig{
GenericDynamicLogLinksEnabled: true,
},
&core.TaskTemplate{
Config: map[string]string{
"link_type": "abc,defg",
"generic_dynamic_log_links": `{"links": [{"name": "abc", "display_name": "Other logs", "template_uri": "coming://from/abc"},{"name": "defg", "display_name": "defg logs", "template_uri": "coming://from/defg"}]}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider splitting long configuration line

Consider splitting the long line containing generic_dynamic_log_links configuration into multiple lines for better readability. The current line exceeds 120 characters which may impact code maintainability.

Code suggestion
Check the AI-generated fix before applying
Suggested change
"generic_dynamic_log_links": `{"links": [{"name": "abc", "display_name": "Other logs", "template_uri": "coming://from/abc"},{"name": "defg", "display_name": "defg logs", "template_uri": "coming://from/defg"}]}`,
"generic_dynamic_log_links": `{
"links": [{"name": "abc", "display_name": "Other logs", "template_uri": "coming://from/abc"},
{"name": "defg", "display_name": "defg logs", "template_uri": "coming://from/defg"}]
}`,

Code Review Run #51871a


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

},
},
[]*core.TaskLog{
{
Uri: "coming://from/abc",
MessageFormat: core.TaskLog_JSON,
Name: "Other logs",
},
{
Uri: "coming://from/defg",
MessageFormat: core.TaskLog_JSON,
Name: "defg logs",
},
},
},
{
"Multiple Generic Dynamic Log Links defined in task template - one defined as a Generic Dynamic Log Link and a regular Dynamic Log Link",
&LogConfig{
GenericDynamicLogLinksEnabled: true,
DynamicLogLinks: map[string]tasklog.TemplateLogPlugin{
"abc": tasklog.TemplateLogPlugin{
DisplayName: "abc link",
TemplateURIs: []tasklog.TemplateURI{
"coming://from/abc",
},
},
},
},
&core.TaskTemplate{
Config: map[string]string{
// Notice that only defg is defined as a generic dynamic log link
"link_type": "abc,defg",
"generic_dynamic_log_links": `{"links": [{"name": "defg", "display_name": "defg logs", "template_uri": "coming://from/defg"}]}`,
},
},
[]*core.TaskLog{
{
Uri: "coming://from/abc",
MessageFormat: core.TaskLog_JSON,
Name: "abc link my-Suffix",
},
{
Uri: "coming://from/defg",
MessageFormat: core.TaskLog_JSON,
Name: "defg logs",
},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertTestSucceeded(t, tt.config, tt.template, tt.expectedTaskLogs, "")
})
}
}
59 changes: 28 additions & 31 deletions flyteplugins/go/tasks/pluginmachinery/core/mocks/task_overrides.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading