bucket inspect: Adds csv, tsv printer for inspect command #4228

someshkoli · 2021-05-12T20:17:55Z

Signed-off-by: someshkoli [email protected]

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Adds csv, tsv printer methods to print block metadata table in csv and tsv format respectively.

Further work:

add json output (decide format)

Closes: #4180

Verification

unit tests needs to be added

wiardvanrij

Thanks for picking this up, awesome 👍

This is just my review/thought;
It's actually really smart and clever, but somewhat hard to follow/read

My initial review would be; what about just passing the *output into the func, like we do with *sortBy and then do a switch compare (default to table).
Then make a sort of "factory"? I.e. printer.Print(data) where printer is the instantiated type of printer depending on the output type?

LMK if this make sense :p

edit:

Example:

thanos/pkg/discovery/dns/provider.go

Line 43 in deb5998

func (t ResolverType) ToResolver(logger log.Logger) ipLookupResolver {

someshkoli · 2021-05-12T20:50:14Z

Thanks for picking this up, awesome

This is just my review/thought;
It's actually really smart and clever, but somewhat hard to follow/read

My initial review would be; what about just passing the *output into the func, like we do with *sortBy and then do a switch compare (default to table).
Then make a sort of "factory"? I.e. printer.Print(data) where printer is the instantiated type of printer depending on the output type?

LMK if this make sense :p

Yeah it does make sense, I initially thought of making an interface for diff printers along with their configs (TSV could have diff tab lengths etc ) but then doubted if i'm over engineering it....

wiardvanrij · 2021-05-12T20:54:45Z

Thanks for picking this up, awesome
This is just my review/thought;
It's actually really smart and clever, but somewhat hard to follow/read
My initial review would be; what about just passing the *output into the func, like we do with *sortBy and then do a switch compare (default to table).
Then make a sort of "factory"? I.e. printer.Print(data) where printer is the instantiated type of printer depending on the output type?
LMK if this make sense :p

Yeah it does make sense, I initially thought of making an interface for diff printers along with their configs (TSV could have diff tab lengths etc ) but then doubted if i'm over engineering it....

That is actually a fair point!
I think it would help with readability though, yet you are right that this functionality is between the lines of over-engineering or not. Regardless, it looks good. If you have any doubts, don't hesitate to ask for more input on slack or something 👍

someshkoli · 2021-05-12T20:57:18Z

Thanks for picking this up, awesome
This is just my review/thought;
It's actually really smart and clever, but somewhat hard to follow/read
My initial review would be; what about just passing the *output into the func, like we do with *sortBy and then do a switch compare (default to table).
Then make a sort of "factory"? I.e. printer.Print(data) where printer is the instantiated type of printer depending on the output type?
LMK if this make sense :p

Yeah it does make sense, I initially thought of making an interface for diff printers along with their configs (TSV could have diff tab lengths etc ) but then doubted if i'm over engineering it....

That is actually a fair point!
I think it would help with readability though, yet you are right that this functionality is between the lines of over-engineering or not. Regardless, it looks good. If you have any doubts, don't hesitate to ask for more input on slack or something

Yes, would be easier to decide if there are some other use cases too

matthiasr

What do the outputs look like, can you show some examples? Do we need to worry about quoting at all, or does encoding/csv do the right thing?

matthiasr · 2021-05-13T10:46:52Z

cmd/thanos/tools_bucket.go

@@ -272,6 +282,9 @@ func registerBucketInspect(app extkingpin.AppClause, objStoreConfig *extflag.Pat
 		Default("FROM", "UNTIL").Enums(inspectColumns...)
 	timeout := cmd.Flag("timeout", "Timeout to download metadata from remote storage").Default("5m").Duration()

+	output := cmd.Flag("output", "Output format for result. Currently supports table, cvs, and, tsv.").Default("table").String()


the sortBy flag seems to use enums to restrict the possible values, is this something that you could do here as well?

matthiasr · 2021-05-13T10:49:34Z

cmd/thanos/tools_bucket.go

@@ -616,7 +636,56 @@ func registerBucketCleanup(app extkingpin.AppClause, objStoreConfig *extflag.Pat
 	})
 }

-func printTable(blockMetas []*metadata.Meta, selectorLabels labels.Labels, sortBy []string) error {
+type tablePrinter func(t Table) error


I'm not sure about hardcoding the output to os.Stdout in all the functions. What do you think about passing an io.Writer into any tablePrinter?

Suggested change

type tablePrinter func(t Table) error

type tablePrinter func(w io.Writer, t Table) error

Yes, even I was sceptical about using stdout directly here.

matthiasr · 2021-05-13T10:51:12Z

docs/components/tools.md

@@ -436,6 +436,8 @@ Flags:
                             Path to YAML file that contains object store
                             configuration. See format details:
                             https://thanos.io/tip/thanos/storage.md/#configuration
+      --output="table"       Output format for result. Currently supports table,
+                             cvs, and, tsv.


Suggested change

cvs, and, tsv.

csv, and tsv.

or maybe make it "technical" and leave out the and altogether?

Suggested change

cvs, and, tsv.

csv, tsv.

or maybe make it "technical" and leave out the and altogether?

this one

GiedriusS · 2021-05-13T15:37:47Z

cmd/thanos/tools_bucket.go

+		} else if opType == CSV {
+			return printBlockData(blockMetas, selectorLabels, *sortBy, printCSV)
+		} else {
+			return fmt.Errorf("Invalid output type %s.", *output)


Typically error messages start with a lower letter because they look properly then in case they get chained e.g. doing: Creating: linking: ... vs. doing: creating: linking: .... In fact, this case would be handled for you if you'd use enums as recommended previously

got it thanks. will keep in mind for future contributions

someshkoli · 2021-05-13T18:27:21Z

What do the outputs look like, can you show some examples? Do we need to worry about quoting at all, or does encoding/csv do the right thing?

Here's the output

And yes encoding/csv is behaving as expected I guess. There are some inconsistencies in tab spacing and I'm not sure why...
Any ideas why it's happening?

yeya24 · 2021-06-30T01:44:39Z

This looks good and I love it. Quick question, does it support outputting csv/tsv to a file? @someshkoli

matthiasr · 2021-06-30T07:41:50Z

What prevents you from redirecting it to a file like thanos bucket inspect … --output=csv > bucket.csv?

someshkoli · 2021-06-30T07:46:05Z

This looks good and I love it. Quick question, does it support outputting csv/tsv to a file? @someshkoli

What prevents you from redirecting it to a file like thanos bucket inspect … --output=csv > bucket.csv?

++, there isn't much work to output it to a file but might be redundant

GiedriusS · 2021-06-30T14:53:18Z

Is this still a draft?

yeya24 · 2021-06-30T16:36:29Z

What prevents you from redirecting it to a file like thanos bucket inspect … --output=csv > bucket.csv?

@matthiasr I am thinking about the logger output. They are also written to stdout and will be mixed with the csv output I guess? An output file flag might be necessary.

someshkoli · 2021-07-01T00:54:20Z

Is this still a draft?

Not really but tsv output is sort of inconsistent, didn't spend too much time on this, will check soon

Signed-off-by: someshkoli <[email protected]>

someshkoli · 2021-07-05T15:46:22Z

Is this still a draft?

Not really but tsv output is sort of inconsistent, didn't spend too much time on this, will check soon

sorry for delayed update
Not sure what this error is but while reading the output via machine code it works properly so ig we're good to go

stale · 2021-09-06T18:30:53Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

matthiasr

This looks good to me. @yeya24 as far as I can see, the logger goes to stderr, so this is no problem – if you redirect stdout you still get logs to the terminal and the data goes to the file.

yeya24 · 2021-10-08T17:29:35Z

This looks good to me. @yeya24 as far as I can see, the logger goes to stderr, so this is no problem – if you redirect stdout you still get logs to the terminal and the data goes to the file.

SGTM. @someshkoli Sorry for the delay. Can you do a rebase and then we can review and merge this?

someshkoli · 2021-10-12T07:28:37Z

This looks good to me. @yeya24 as far as I can see, the logger goes to stderr, so this is no problem – if you redirect stdout you still get logs to the terminal and the data goes to the file.

SGTM. @someshkoli Sorry for the delay. Can you do a rebase and then we can review and merge this?

will update the pr in a day or two, a bit occupied lately

yeya24 · 2021-11-08T18:54:09Z

@someshkoli I would like to continue this if you are busy with other work so please let me know 😄

someshkoli · 2021-11-08T18:55:52Z

@someshkoli I would like to continue this if you are busy with other work so please let me know smile

Will wrap this up, rebasing it. @yeya24 a review ?

yeya24 · 2021-11-08T19:27:20Z

@someshkoli I would like to continue this if you are busy with other work so please let me know smile

Will wrap this up, rebasing it. @yeya24 a review ?

I don't think this works. Can you fix go build?

someshkoli · 2021-11-08T19:28:54Z

@someshkoli I would like to continue this if you are busy with other work so please let me know smile

Will wrap this up, rebasing it. @yeya24 a review ?

I don't think this works. Can you fix go build?

Has become too old :""), ton of rebasing required. Moving this to a clean new PR

yeya24 · 2021-11-08T20:12:27Z

@someshkoli Can you please also update the changelog?

Signed-off-by: someshkoli <[email protected]>

someshkoli · 2021-11-08T21:08:51Z

@someshkoli Can you please also update the changelog?

@yeya24 Done👌

yeya24

LGTM

wiardvanrij reviewed May 12, 2021

View reviewed changes

matthiasr reviewed May 13, 2021

View reviewed changes

GiedriusS reviewed May 13, 2021

View reviewed changes

someshkoli force-pushed the feature/bucket-inspect-output branch from 758efcd to ae26d9e Compare May 13, 2021 18:10

someshkoli requested a review from GiedriusS May 14, 2021 09:35

someshkoli marked this pull request as ready for review July 5, 2021 15:42

someshkoli added 8 commits July 5, 2021 21:13

Adds csv, tsv printer and updates table printer

3c8f088

Signed-off-by: someshkoli <[email protected]>

updates docs

b69c0a7

Signed-off-by: someshkoli <[email protected]>

adds err handling to csv write

73e3a28

Signed-off-by: someshkoli <[email protected]>

adds err handling to invalid outputtype

8a1df54

Signed-off-by: someshkoli <[email protected]>

adds err handling to invalid outputtype

ebb77f2

Signed-off-by: someshkoli <[email protected]>

adds new tsv writer

f50d2ba

Signed-off-by: someshkoli <[email protected]>

updates docs

7be7696

Signed-off-by: someshkoli <[email protected]>

added additional prram w to writer

639c8cc

Signed-off-by: someshkoli <[email protected]>

someshkoli force-pushed the feature/bucket-inspect-output branch from a953792 to 639c8cc Compare July 5, 2021 15:44

stale bot added the stale label Sep 6, 2021

stale bot closed this Sep 19, 2021

matthiasr reviewed Oct 8, 2021

View reviewed changes

GiedriusS reopened this Oct 8, 2021

stale bot removed the stale label Oct 8, 2021

someshkoli force-pushed the feature/bucket-inspect-output branch from 2d18563 to 639c8cc Compare November 8, 2021 19:58

someshkoli force-pushed the feature/bucket-inspect-output branch from cab9533 to 639c8cc Compare November 8, 2021 20:39

someshkoli added 2 commits November 9, 2021 02:15

update docs and changelog

b040d74

Signed-off-by: someshkoli <[email protected]>

Merge branch 'main' into feature/bucket-inspect-output

28f7c07

yeya24 approved these changes Nov 8, 2021

View reviewed changes

yeya24 enabled auto-merge (squash) November 8, 2021 21:10

yeya24 merged commit dcbd1f7 into thanos-io:main Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bucket inspect: Adds csv, tsv printer for inspect command #4228

bucket inspect: Adds csv, tsv printer for inspect command #4228

someshkoli commented May 12, 2021 •

edited by yeya24

Loading

wiardvanrij left a comment •

edited

Loading

someshkoli commented May 12, 2021

wiardvanrij commented May 12, 2021

someshkoli commented May 12, 2021 •

edited

Loading

matthiasr left a comment

matthiasr May 13, 2021

matthiasr May 13, 2021

someshkoli May 13, 2021 •

edited

Loading

matthiasr May 13, 2021

matthiasr May 13, 2021

someshkoli May 13, 2021

GiedriusS May 13, 2021

someshkoli May 13, 2021

someshkoli commented May 13, 2021 •

edited

Loading

yeya24 commented Jun 30, 2021 •

edited

Loading

matthiasr commented Jun 30, 2021

someshkoli commented Jun 30, 2021

GiedriusS commented Jun 30, 2021

yeya24 commented Jun 30, 2021

someshkoli commented Jul 1, 2021

someshkoli commented Jul 5, 2021

stale bot commented Sep 6, 2021

matthiasr left a comment

yeya24 commented Oct 8, 2021

someshkoli commented Oct 12, 2021

yeya24 commented Nov 8, 2021

someshkoli commented Nov 8, 2021 •

edited

Loading

yeya24 commented Nov 8, 2021

someshkoli commented Nov 8, 2021

yeya24 commented Nov 8, 2021

someshkoli commented Nov 8, 2021 •

edited

Loading

yeya24 left a comment

	type tablePrinter func(t Table) error
	type tablePrinter func(w io.Writer, t Table) error

bucket inspect: Adds csv, tsv printer for inspect command #4228

bucket inspect: Adds csv, tsv printer for inspect command #4228

Conversation

someshkoli commented May 12, 2021 • edited by yeya24 Loading

Changes

Verification

wiardvanrij left a comment • edited Loading

Choose a reason for hiding this comment

someshkoli commented May 12, 2021

wiardvanrij commented May 12, 2021

someshkoli commented May 12, 2021 • edited Loading

matthiasr left a comment

Choose a reason for hiding this comment

matthiasr May 13, 2021

Choose a reason for hiding this comment

matthiasr May 13, 2021

Choose a reason for hiding this comment

someshkoli May 13, 2021 • edited Loading

Choose a reason for hiding this comment

matthiasr May 13, 2021

Choose a reason for hiding this comment

matthiasr May 13, 2021

Choose a reason for hiding this comment

someshkoli May 13, 2021

Choose a reason for hiding this comment

GiedriusS May 13, 2021

Choose a reason for hiding this comment

someshkoli May 13, 2021

Choose a reason for hiding this comment

someshkoli commented May 13, 2021 • edited Loading

yeya24 commented Jun 30, 2021 • edited Loading

matthiasr commented Jun 30, 2021

someshkoli commented Jun 30, 2021

GiedriusS commented Jun 30, 2021

yeya24 commented Jun 30, 2021

someshkoli commented Jul 1, 2021

someshkoli commented Jul 5, 2021

stale bot commented Sep 6, 2021

matthiasr left a comment

Choose a reason for hiding this comment

yeya24 commented Oct 8, 2021

someshkoli commented Oct 12, 2021

yeya24 commented Nov 8, 2021

someshkoli commented Nov 8, 2021 • edited Loading

yeya24 commented Nov 8, 2021

someshkoli commented Nov 8, 2021

yeya24 commented Nov 8, 2021

someshkoli commented Nov 8, 2021 • edited Loading

yeya24 left a comment

Choose a reason for hiding this comment

someshkoli commented May 12, 2021 •

edited by yeya24

Loading

wiardvanrij left a comment •

edited

Loading

someshkoli commented May 12, 2021 •

edited

Loading

someshkoli May 13, 2021 •

edited

Loading

someshkoli commented May 13, 2021 •

edited

Loading

yeya24 commented Jun 30, 2021 •

edited

Loading

someshkoli commented Nov 8, 2021 •

edited

Loading

someshkoli commented Nov 8, 2021 •

edited

Loading