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

Fix another variant blank log lines #2959

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stuartc
Copy link
Member

@stuartc stuartc commented Feb 21, 2025

Description

Re: #2914

Been seeing a bunch of warnings on Sentry saying that a log line couldn't be blank/is rejected.

Really thought I fixed this one.

From what it looks like, console.log(undefined) arrives as %{"message": []} which was previously considered blank.

This PR converts drops [] from the list of 'empty values' when casting, which is valid (from the previous set of changes on this issue).

Now the question is here, is there such a thing as a log line that is "too blank"?

> console.log()

undefined
> console.log(undefined)
undefined
undefined

So in the node real, an empty console.log, really logs an empty string, and logging undefined prints undefined.

Perhaps this should be changed upstream in the worker, either actually send ['undefined'] or skip sending the message?

Validation steps

See: test/integration/web_and_worker_test.exs:290, and run it with mix test.watch test/integration/web_and_worker_test.exs:290.

This test checks the different output casting for log lines.

Additional notes for the reviewer

  1. (Is there anything else the reviewer should know or look out for?)

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed a self-review of my code.
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@stuartc stuartc self-assigned this Feb 21, 2025
@stuartc stuartc requested a review from josephjclark February 21, 2025 13:52
@josephjclark
Copy link
Contributor

@stuartc something I do a lot in the CLI, to get better log output, is console.log(). Just to get the clean line break and make logs more readable in stdout. But this stuff isn't useful at all for Lightning, where ideally we'd have a nice logger UI which lets us filter stuff and there's natural padding of lines so we don't need artificial breaks.

This is different of course to a user (or maybe an adaptor) doing console.log(x) where x is undefined.

I could look into a few things upstream:

  • In the job logger - the thing that logs user's console.log statements - I could (probably) detect an undefined value and pass an 'undefined' string instead. Now Lightning will receive message: ['undefined']. I don't know if that will confuse anything. I suppose I could also do message: [undefined] if an argument was passed, which is probably more useful
  • The logger has console.break() function, which I think is mostly what the CLI uses for this. I could have a rule like: when logging json output, ignore any console.break statements - because we assume that the JSON will be consumed by something richer that a terminal downstream.
  • The worker could refuse to send any logs with message: [] on the assumption that it's a waste of everyone's time

If I do that, then I think I can promise:

  • message: [undefined] is useful and even important to log
  • You will not be sent meaningless empty lines which are just stdout padding

@josephjclark
Copy link
Contributor

From stu: stringifiying console.log(undefined) would help downstream

@josephjclark
Copy link
Contributor

bottom line: the contract on log is that the worker always sends a message with readable content. No empty lines. Stringified undefineds. Only send it if it has value to lightning.

I should extend the mock to throw for empty lines too, to help us catch this stuff

@josephjclark josephjclark self-assigned this Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants