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

Log template errors as debug message #2637

Closed
wants to merge 9 commits into from

Conversation

JoannaaKL
Copy link
Contributor

@JoannaaKL JoannaaKL commented Jun 1, 2023

Background
When parsing a workflow template there are two objects at play: ExecutionContext and TemplateContext. ExecutionContext stores all errors (named issues) that are later sent as job results. TemplateContext on the other side is an object used, as the name suggests, during the template parsing. It also stores template errors together with field ids and names, to provide a meaningful message, which workflow line was invalid.

The duplication comes from the fact that during the template reading:

  1. first we’re loading the template using TemplateReader.Read and here template errors are being written to the TemplateContext for the first time
  2. after we’re done with loading all tokens we go through all TemplateContext errors and add them to the ExecutionContext errors list here (screenshot with more logs is at the bottom in expandable section)

All that would be fine if not the small fact, that in step 1 TemplateContext already added errors to the ExecutionContext: internally it is using TraceWriter.Error method that is…writing errors to the ExecutionContext:

internal void Error(TemplateValidationError error)
{
            Errors.Add(error); <——— add error to Template errors (we iterate over them in step 2) 
            TraceWriter.Error(error.Message); <—— TraceWriter writes errors to ExecutionContext directly causing duplication 
}

link

We could deduplicate issues in ExecutionContext based on error position and message, but that would not fix the underlying issue of logging two times the same error or fix the real issue, that is writing the same errors in multiple places.
Since the workflow parser code is used in other classes and there's a risk of regression, I have added a feature flag DistributedTask.LogTemplateErrorsToTraceWriter that will be passed from actions service. This way only one method in TemplateReader will stop duplicating errors. With time we can expand this logic and disable TraceWriter error logging in other places.
Long term solution would be code a refactoring, since TemplateContext has access to the TraceWriter, it can manipulate the state of ExecutionContext. These two should either be separated or merged into one object.

Closes https://github.com/github/c2c-actions-runtime/issues/2381
Follow-up: https://github.com/github/actions-dotnet/pull/15290

Before
Screenshot 2023-05-31 at 16 01 33
After
Screenshot 2023-05-31 at 16 01 05

Workflow run with more debug logs

Branch

Screenshot 2023-06-13 at 09 03 12

@JoannaaKL JoannaaKL marked this pull request as ready for review June 1, 2023 14:53
@JoannaaKL JoannaaKL requested a review from a team as a code owner June 1, 2023 14:53
@JoannaaKL JoannaaKL closed this Jun 2, 2023
@JoannaaKL JoannaaKL reopened this Jun 2, 2023
@JoannaaKL JoannaaKL force-pushed the LogTemplateErrorsAsDebugMessage branch from 76c3ae0 to 2be59d6 Compare June 2, 2023 11:24
Copy link
Member

Choose a reason for hiding this comment

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

Try to not change anything under src/Sdk within the runner, these files are shared with the service and they should be keep in sync. If you really plan to change this file, you will need to change it on the service first and rollout out to production, then copy the changed version to the runner.

Copy link
Member

Choose a reason for hiding this comment

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

Undo change in this file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants