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

Write informational output to stderr for tail and search #43

Merged
4 commits merged into from
May 12, 2020
Merged

Conversation

ghost
Copy link

@ghost ghost commented May 6, 2020

Plaintext summary lines, 'no result' lines, and internal errors are sent to stderr to not interfere with actual data for tail and search.
This also fixes #42 where piping output from tail and search under json flag would break due to summary line.

Before

logdna search "docker" -j | jq
parse error: Invalid numeric literal at line 1, column 7

After

logdna search "docker" -j | jq
search finished: 5000 line(s). hosts: all. apps: all. levels: all. tags: all. query: docker
{
  ...json output
}

@ghost ghost requested review from smusali and matt-march May 6, 2020 22:10
@smusali smusali assigned ghost May 7, 2020
@jakedipity
Copy link
Contributor

utils.log seems to have some handling when console.log fails. I'm assuming that was added on purpose due to some issue, so I think we should try to respect that as well.

@smusali smusali requested a review from dm36 May 7, 2020 18:37
@ghost
Copy link
Author

ghost commented May 8, 2020

fixed

Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

I don't seem to understand why we are printing informational statements out to stderr for just solving non-JSON summary line interference; there are multiple other options:

  • making the summary line itself a JSON since it has <key>: <value> pairs
  • or with an extra parameter, making it disappear or appear, with something like --quiet or --noSummary

@smusali
Copy link
Contributor

smusali commented May 8, 2020

@ydshah2, can you mention this PR (#43) in #42 by saying this PR is intended to solve the issue? Thanks a lot in advance!

@ghost
Copy link
Author

ghost commented May 8, 2020

Adding discussions with @matt-march and @andkon, the change is not just to fix #42. That is what started the discussion. It is a move towards good design across the board. We are also keeping the stderr for internal output for both json and regular usecase for the sake of consistency. It is in line with what tools like curl do - something people are already familiar with. This will be a breaking change, and the version change will reflect that but the idea is to just do it the right way rather than add a flag just to satisfy the issue or for not being breaking.

@ghost ghost force-pushed the yash_jsonFix branch from 558db8f to b400366 Compare May 8, 2020 20:43
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

Okay, just 2 requests:

  • including console.error addition into utils.log instead of creating utils.error
  • adding unit test cases for utils.log for now

Since we have never had unit tests for LogDNA CLI, we can start from here, at least, and then expand.

lib/utils.js Outdated Show resolved Hide resolved
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

small requests again

lib/utils.js Outdated Show resolved Hide resolved
lib/utils.js Outdated Show resolved Hide resolved
@ghost ghost force-pushed the yash_jsonFix branch from d008eb9 to 18b12f1 Compare May 12, 2020 17:43
@smusali smusali self-requested a review May 12, 2020 17:52
smusali
smusali previously approved these changes May 12, 2020
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

lgtm!

@smusali smusali self-requested a review May 12, 2020 21:17
Copy link
Contributor

@smusali smusali left a comment

Choose a reason for hiding this comment

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

loved very last change

@ghost ghost merged commit 5de6534 into master May 12, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable option to get truly raw output in searches
2 participants