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

feat: provide logger customization #410

Closed
wants to merge 5 commits into from
Closed

Conversation

antongolub
Copy link
Collaborator

@antongolub antongolub commented May 27, 2022

closes #376
closes #405

  • Tests pass
  • Appropriate changes to README are included in PR
  • Types updated
test('logger works', async () => {
  let stdout = ''
  let stderr = ''
  $.logFormat = (msg) => msg.map((m) => m.toUpperCase())
  $.logPrint = (data, err) => {
    if (data) stdout += data
    if (err) stderr += err
  }
  $.logIgnore = ['b*', 'fetch']
  // NOTE first arg declares scope
  log('foo', 'foo-test')
  log('bar', 'bar-test')
  log('baz', 'baz-test')
  log('fetch', 'example.com')

  assert(stdout.includes('FOO-TEST'))
  assert(!stdout.includes('BAR-TEST'))
  assert(!stdout.includes('BAZ-TEST'))
  assert(!stdout.includes('EXAMPLE.COM'))

  $.logOutput = 'stderr'
  log('foo', 'foo')
  assert(stderr.includes('FOO'))

  delete $.logPrint
  delete $.logFormat
  delete $.logIgnore
  delete $.logOutput
})

ignore dep already comes with globby.

image

Pros

  • filtering events by their scope: cd/fetch/cmd,...
  • single entry point to introduce creds concealer
  • custom formatting to attach mdc, timestamps, etc
  • logging rules are attached to current ctx, so they can be applied to a cmd block.
  • applied to spawned processes outputs
  • if (!verbose) return at single place

Cons

  • inherits verbose flag
  • too imperative, +4 config options
  • not applied to spawned processes outputs, they just go as is to the root process stream

@antongolub antongolub changed the title feat: add logger customization feat: provide logger customization May 27, 2022
@antongolub
Copy link
Collaborator Author

antongolub commented May 27, 2022

@JohnnyMarnell, @ryami333,

Let's discuss if it covers your issues.

@antonmedv
Copy link
Collaborator

What about removing log from fetch at all? This will be more “browser” like.

@antonmedv
Copy link
Collaborator

4 additional API endpoints) I’ll try to do my version of addressing same issues today.

@antongolub
Copy link
Collaborator Author

antongolub commented May 27, 2022

browser

zx is suitable for crawling too, so fetch/http logging might be useful.

4 additional API endpoints)

I can assemble them into $.log.*. But that will be cheating)

I’ll try to do my version of addressing same issues today.

It's essential to bind log profile to async ctx:

$.profile({logFormat: maskingLogger}, async() => {
  await $.fetch('https://exampe.com', {authorization: ...})
  // fetch https://exampe.com {"Authorization": "23e334ds******23"}
  $`qctl login ${token}`
  // $ qctl login fr324d324******09 
})

$.profile({logIgnore: 'fetch'}, () => {
   // push smth optinal that does not requre logging or even waiting
  for(let url of urls) {
     $.fetch(url, {data: ...})
   }
})

@antonmedv
Copy link
Collaborator

zx is suitable for crawling too, so fetch/http logging might be useful.

True.

I’m thinking about something like this:

  • true (dedault) - logs everything, to stderr
  • cmd only (fetch can be treated as cmd) - logs only commands, no outputs
  • false - no logging.

@antongolub
Copy link
Collaborator Author

default stderr

+ zx logs are more of debug/tracing which usually goes to stderr (console.trace)

@antonmedv
Copy link
Collaborator

Also echo can also be to stderr. WDYT?

@antongolub
Copy link
Collaborator Author

antongolub commented May 27, 2022

cmd only (fetch can be treated as cmd) - logs only commands, no outputs

Let's make it configurable like in debug:

log('cmd:input', ...)
log('cmd:output', ...)

$.logIgnore = ['cmd:*'] // to disable all
$.logIgnore = ['cmd:output'] // to disable stdout pushing

External plugins may bring their own log events, so we need to apply filters to them too.

@antongolub
Copy link
Collaborator Author

antongolub commented May 27, 2022

Also echo can also be to stderr

This might be surprising a bit.

UPD But if logOutput set to stderr this will happen )

@antonmedv
Copy link
Collaborator

Lets not allow plugins to use zx logs. I’d like to keep logs of zx as internal as possible.

@antongolub
Copy link
Collaborator Author

antongolub commented May 27, 2022

Logger is essential.

  1. We can introduce protected scope zx and restrict plugins from using it: zx:cmd:input, zx:fetch:input, etc.
  2. It's immutable, I don't see any risks

Why log format is needed: we have a special magic process on our kuber pods that captures stdstreams and pushes their contents to elastic index. But it happens only for json strings with special fields, so we have to attach them somehow.

@antongolub
Copy link
Collaborator Author

antongolub commented May 27, 2022

@antonmedv

Show your concept. I'll try to mimic. 0/1/2 level schema as you mentioned is fine, but it has some limitations. For large scripts level 1 may be not enough for debugging when option 0 produces too big output to recognize.

Instead, I suggest introducing a composite dynamic scope prefix: zx:cmd:input:${cmd} so we could even filter only necessary commands. Maximum granularity

@antongolub
Copy link
Collaborator Author

Idea! We can use standard env.DEBUG value as logIgnore default if specified.

@antonmedv
Copy link
Collaborator

zx:cmd:input:${cmd}

too many abstraction. Lets try to find simplest solution. Let’s reduce complexity. Can you expla little bit more your needs?

@antongolub
Copy link
Collaborator Author

antongolub commented May 27, 2022

too many abstraction

Too many java devs around ))) I'm trying to attach meta to every log event to be able to: 1. process only necessary, 2. define event projection — format and channel

How I'm going to use these powers — wrapping some code blocks with retries and raising log verbosity for the last attempt and every next matching bin.

@antongolub
Copy link
Collaborator Author

Maybe it's too much. Reverted.

@JohnnyMarnell
Copy link

My biggest complaint was the nonstandard usage of stdout for diagnostic logging, rendering any shell script looking to provide an "output" problematic. Seeing that's finally being defaulted to stderr recently, great. Beyond that, the logging mechanism doesn't need to be this complex, a simple -v vs -vvvvv verbosity level mechanism would have sufficed. But main thing is if it has sane defaults in addition to stderr, which I think this does...

@antongolub
Copy link
Collaborator Author

antongolub commented May 28, 2022

Thanks for the feedback. I've tried to combine both approaches:

  • verbosity level describes "importance"
  • scope marks the event source (fetch, cmd, etc)

@antongolub antongolub force-pushed the logger branch 2 times, most recently from 0623812 to 17b9889 Compare May 30, 2022 21:13
@antongolub antongolub mentioned this pull request May 31, 2022
3 tasks
@antonmedv
Copy link
Collaborator

The v7 will introduce $.log. Closing in flavor of.

@antonmedv antonmedv closed this Jun 6, 2022
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.

How to make "quiet" fetch request?
3 participants