turbo icon indicating copy to clipboard operation
turbo copied to clipboard

Add new option for flag 'output-logs'

Open sprohaszka opened this issue 2 years ago • 11 comments

Here is a quick proposition to solve issue 901.

Not sure this is the correct way to do it. Don't hesitate to tell me, I will update the branch :)

Edit

Add a --raw flag to strip all prefix from outputs.

Remove all logic for prefix management from logstreamer. Centralize prefix management inside runcache (not the best choice in terms of architecture, but the quickest for the time being) as it is the key entry point for lot of write logic.

sprohaszka avatar Jun 08 '22 15:06 sprohaszka

@sprohaszka is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Jun 08 '22 15:06 vercel[bot]

I wonder if this might be better suited for its own flag? This pull request adds "full-no-prefix" as a flag option, but, I could see this feature being useful for "new-only" as well. In the interest of not having a "-no-prefix" version of every option, do you think a boolean "--output-log-prefix" or "--prefix-output-log" flag might fit better?

ObliviousHarmony avatar Jun 08 '22 18:06 ObliviousHarmony

I think the big challenge with #901 is that currently, the prefix (and color!) are baked into the log files that are cached. So while this option would prevent that on a particular run, a cache hit from a run that did not include the flag would still have prefixes. And similarly, a later cache hit of this run would not have prefixes, regardless of whether or not this flag was specified.

I think the way to go about this is to:

  • stop adding prefixes and colors to the log files by default
  • update the global hash key to force cache misses, given that our cache structure will have changed
  • add prefixes and colors back by default when streaming the logs
  • take @ObliviousHarmony's suggestion and have a new flag that controls whether or not to add prefixes and colors during log replay as well as normal execution (--raw? or something like that?)

One exception to the last point may be if we for some reason want to control prefixes based on whether or not there was a cache hit. Even then, we might want to look at extending the values accepted by the new flag, rather than doing the multiplication of all of the log output modes times the prefix options.

gsoltis avatar Jun 08 '22 20:06 gsoltis

Ok. Got it. I will try to propose something new today or tomorrow.

sprohaszka avatar Jun 14 '22 08:06 sprohaszka

It is maybe not the best option to put all prefix and its color inside TaskCache. I aim to put everything in one unique place. Maybe we can improve this in a second PR. What do you think?

sprohaszka avatar Jun 15 '22 16:06 sprohaszka

Hi @gsoltis. Have I some improvement to do on this PR? Did you change your mind about the interest of this feature?

sprohaszka avatar Jun 27 '22 12:06 sprohaszka

@sprohaszka sorry for the delay, I'll take a look at it today

gsoltis avatar Jun 27 '22 15:06 gsoltis

@sprohaszka can you resolve the merge conflicts, and then we can get this reviewed?

gsoltis avatar Aug 17 '22 18:08 gsoltis

@sprohaszka can you resolve the merge conflicts, and then we can get this reviewed?

@gsoltis it is done. If there is any CI error, I will fix them.

sprohaszka avatar Aug 25 '22 09:08 sprohaszka

This PR would help a lot for preserving Github Annotations for linting and typescript errors.

zomars avatar Aug 25 '22 22:08 zomars

@gsoltis Sorry, but I do not understand you about the hit cache. Do you mean that the same flag should have 2 different effects? Can you give an hint on where should the code be modified for the cache hit?

If I correctly understand the existing code, the cache hit line is always written on both cache file and standard output. You want to remove the info from the standard output?

sprohaszka avatar Sep 02 '22 15:09 sprohaszka