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

Dynamic log links #4774

Merged
merged 20 commits into from
Jan 29, 2024
Merged

Dynamic log links #4774

merged 20 commits into from
Jan 29, 2024

Conversation

eapolinario
Copy link
Contributor

@eapolinario eapolinario commented Jan 24, 2024

Tracking issue

Closes #4773

Why are the changes needed?

We need a way to specify dynamic log links from tasks as defined in the linked tracking issue. This PR implements this idea. Essentially operators are going to add a section to their config files for specific dynamic log links. For example, we have flyin as a plugin that would benefit from this feature.

What changes were proposed in this pull request?

We add a new subsection to the plugin section in the propeller config file that lets users specify dynamic plugins. The template uris for these dynamic plugins can be configured similarly to the usual plugins, the only difference that specific fields can be plucked from the task template to be used in the template uri. Take this example:

plugins:
  logs:
    dynamic-log-links-enabled: true
    dynamic-log-links:
      vscode: http://flyteinteractve.vs:{{ .taskConfig.port }}/{{ .podName }}

Notice how in the template uri for the vscode dynamic plugin we access the port (which is defined in a task template metadata) using the prefix .taskConfig, whereas the usual template variables are also available.

How was this patch tested?

Unit tests and tested locally in a sandbox.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

This reverts commit 3501675.

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Jan 24, 2024
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (73b1d40) 58.21% compared to head (d013b31) 58.56%.
Report is 16 commits behind head on master.

Files Patch % Lines
...ugins/go/tasks/pluginmachinery/tasklog/template.go 88.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4774      +/-   ##
==========================================
+ Coverage   58.21%   58.56%   +0.35%     
==========================================
  Files         626      624       -2     
  Lines       53855    53627     -228     
==========================================
+ Hits        31349    31404      +55     
+ Misses      19996    19705     -291     
- Partials     2510     2518       +8     
Flag Coverage Δ
unittests 58.56% <94.23%> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eapolinario
Copy link
Contributor Author

eapolinario commented Jan 27, 2024

@EngHabu , as we talked about yesterday, this next batch of updates implements:

  • remove enabled flag
  • do not hardcode port
    • In fact, we now let users match any value set in the task template config
  • Use vscode as the name of the log
  • Add name to TemplateLogPlugin to help distinguish which plugin is used
    • Remove =DynamicTemplateURI=
  • Get rid of Schemes

flyteplugins/go/tasks/pluginmachinery/tasklog/template.go Outdated Show resolved Hide resolved
flyteplugins/go/tasks/pluginmachinery/tasklog/template.go Outdated Show resolved Hide resolved
flyteplugins/go/tasks/pluginmachinery/tasklog/template.go Outdated Show resolved Hide resolved
tasklog.TemplateLogPlugin{
Name: logLinkType,
DisplayName: fmt.Sprintf("%s logs", logLinkType),
DynamicTemplateURIs: []tasklog.TemplateURI{
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this field still needed? can't we use TemplateURIs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This simplifies the distinction between task logs produced by dynamic log links vs regular log links.

For example, let's say we enable one dynamic log link in propeller. How do we make sure that dynamic log links only show up for tasks that have the right bits set in their task templates?

EngHabu
EngHabu previously approved these changes Jan 29, 2024
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Thank you ❤️ ❤️

Name: logLinkType,
DisplayName: dynamicLogLink.DisplayName,
DynamicTemplateURIs: dynamicLogLink.TemplateURIs,
MessageFormat: core.TaskLog_JSON,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it not have a MessageFormat field?

flyteplugins/go/tasks/pluginmachinery/tasklog/template.go Outdated Show resolved Hide resolved
for _, dynamicLogLinkType := range getDynamicLogLinkTypes(input.TaskTemplate) {
for _, dynamicTemplateURI := range p.DynamicTemplateURIs {
if p.Name == dynamicLogLinkType {
for _, match := range dynamicLogRegex.FindAllStringSubmatch(dynamicTemplateURI, -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would be nice to support this for the "regular" logs as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be easy to rearrange this logic when we find a good use-case for regular logs.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 29, 2024
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario eapolinario merged commit 460f0f1 into master Jan 29, 2024
45 checks passed
@eapolinario eapolinario deleted the dynamic-log-links branch January 29, 2024 22:55
pmahindrakar-oss pushed a commit that referenced this pull request May 1, 2024
* Dynamic log links (#4774)

* Bring Scheme back for backwards compatibility (#4789)

* Bring Scheme back for backwards compatibility

Signed-off-by: Eduardo Apolinario <[email protected]>

* Rename deprecated field to `DeprecatedScheme`

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core feature] Dynamic log links
3 participants