feat(cmd/influx/write): enhance the ability to import CSV files

[sranka]

Closes #17004 #17008

There is a bunch of enhancements in the way of how CSV data are processed by influx write so that external CSV data can be written to InfluxDB. They are described in detail in #17004 + #17008

Examples of new capabilities are provided in https://github.com/bonitoo-io/influxdb-csv-import/blob/master/README.md#step-2

New flags in influx write :

      --debug                Log CSV columns to stderr before reading data rows
      --encoding string      Character encoding of input files or stdin (default "UTF-8")
  -f, --file stringArray     The path to the file to import
      --header stringArray   One or more header lines to prepend to input data
      --skipHeader int[=1]   Skip the first <n> rows from input data
      --skipRowOnError       Log CSV data errors to stderr and continue with CSV processing

• CHANGELOG.md updated with a link to the PR (not the Issue)
• Well-formatted commit messages
• Rebased/mergeable
• Tests pass
• Documentation updated or issue created (provide link to issue/pr)
• Signed CLA (if not already signed)

[jsteenb2]

we need a reivew from @influxdata/storage-team here as well

[jsteenb2]

what is the motivation behind this? can we remove it?

[sranka]

It helps with the troubleshooting of CSV -> LP conversion. It is easier to find problems in user input when you dump the actual input arguments together with their default values. This is printed only in --debug mode, together with metadata about columns that are used to extract CSV data that create protocol lines.

Sure I can remove it, but it will be then harder to resolve user input errors. Opinionated herein, I do not insist on having it here.

[jsteenb2]

why are readers and closers separate instead of being ReadClosers?

[sranka]

closers are subset of readers herein, only opened files will be closed, nothing more ... stdin or string readers cannot/must not be closed.

[jsteenb2]

I hear you, seems better to do it as ReadClosers with a noop closer on those that don't need a closer, typical pattern in golang.

type noopCloser struct {
    r io.reader
}

func (n *noopCloser) Close() error {
    return nil 
}

my big concern here is it makes it easy to lose track of closers/readers when they ares disparate uncoupled return types.

Last thing, please don't resolve conversations, I'll happily resolve them when I feel the conversation has come to fruition 👍

[sranka]

Thank you for the clarification around "conversation resolving", as a newbie herein, I need such advice 👍

I see two concerns herein:

1. Why are readers and closers separate instead of being ReadClosers?
   • files can be provided in non-UTF-8 encoding, a file (ReaderCloser) is then transformed to UTF-8 Reader (not a ReadCloser)
     

     influxdb/cmd/influx/write.go

     Line 141 in b786fc1

     readers = append(readers, decode(f), strings.NewReader("\n"))

   • the last file or stdin can be then changed so that first N lines are skipped when --skipHeader option is present. This transformation also works with Readers, but not ReadClosers

     influxdb/cmd/influx/write.go

     Line 168 in b786fc1

     readers[i] = write.SkipHeaderLinesReader(writeFlags.SkipHeader, readers[i])

   • changing the above intermediate Readers to ReadClosers to have just one ReadCloser array would introduce an extra complexity herein IMO
2. Why are io.Reader and io.Closer disparate return values?
   • I understand that a composite io.ReaderCloser looks better. It would require to wrap multiple Readers and Closers (or ReadClosers) in a new type that will only delegate calls. I wanted to prefer builtins whenever possible, but there is no multi-reader-closer for reuse.
   • At the same time, it is not a big deal (in my POV) for a non-public function to avoid this wrapping. "A returned Closer must be closed" is an expected contract IMO.

[jsteenb2]

I'm concerned the flags are being used as a bucket for a bunch of side effects here

[sranka]

yes, for the sake of backward compatibility @file means a file ... somebody introduced this and I decided to keep this functionality because I don't know where and how it is currently used. When starting from scratch, I would not allow specifying data in an argument at all.

[jsteenb2]

I don't htink my eariler comment was clear. I'd rather see you maintain the files in a sep variable instead of creating a destructive call on the flags. Flags should only be used (imo), for the API contract between CLI and user. Makes things coupled in weird global ways atm.

[sranka]

understood, repaired

[jsteenb2]

what is the motivation to move this to the internal pkg?

[sranka]

My motivation: This fn is independent on existing cli commands. I did not convince myself that attaching it to write.go or main.go of the main package is better. I will accept any suggestion herein to follow the mindset of this repo.

[jsteenb2]

I hear you, seems better to do it as ReadClosers with a noop closer on those that don't need a closer, typical pattern in golang.

type noopCloser struct {
    r io.reader
}

func (n *noopCloser) Close() error {
    return nil 
}

my big concern here is it makes it easy to lose track of closers/readers when they ares disparate uncoupled return types.

Last thing, please don't resolve conversations, I'll happily resolve them when I feel the conversation has come to fruition 👍

[jsteenb2]

I don't htink my eariler comment was clear. I'd rather see you maintain the files in a sep variable instead of creating a destructive call on the flags. Flags should only be used (imo), for the API contract between CLI and user. Makes things coupled in weird global ways atm.

[jsteenb2]

just realized this can be simplified too:

return (info.Mode() & os.ModeCharDevice) == os.ModeCharDevice

the if isn't necessary here

[sranka]

ok, simplified

[jsteenb2]

are these sub tests? its easier to erad, but feels like a lot of coupling between assertions. Can this be split up into sub tests?

t.Run("validates no bucket found", func(t *testing.T) {
       lineData := lineData[:0]
        command := cmdWrite(&globalFlags{}, genericCLIOpts{w: ioutil.Discard})
	// note: my-empty-org parameter causes the test server to return no buckets
	command.SetArgs([]string{"--format", "csv", "--org", "my-empty-org", "--bucket", "my-bucket"})
	err = :command.Execute()
	require.Contains(t, fmt.Sprintf("%s", err), "bucket") // no such bucket found
})

same goes for a lot of these assertions here. They look like sub tests to me but aren't being treated as such. Please correct me if that assumption is wrong 🤔

[sranka]

Yes, it looks better when changed to sub-tests

[rbetts]

I have some concerns about this PR. We like the user experience it creates but we aren't comfortable accepting this PR as is because of concerns around complexity and maintainability.

I'd like to see this PR broken into at least two pieces:

1. A standalone CSV to line-protocol conversion library. This should be a reusable library that we can use uniformly in other areas where we want to convert CSV to line protocol.

2. A PR that then uses that library's public API from the CLI command tool to accomplish this CLI workflow.

Separately - I'm curious if there are existing parsers that accomplish the parsing work done here. I don't really want to take on the long term maintenance costs of CSV parsing features. I'm not familiar with what's available in the go language space... But I'm curious if there are third-party CSV processors that would accomplish more with less custom code than is used here.

[sranka]

@rbetts I've seen only a few golang's CSV parser extensions that can do (un)marshaling into structs of a pre-defined layout. We have a different situation herein:

• the layout is driven by CSV data itself with the help of annotations and/or column headers
• a CSV with variable row size (such as a result of a flux query) is supported and can be used on input
• the code should be optimized to transform to line protocol and recognize CSV data annotations that are AFAIK used only in InfluxDB

@russorat Can you please comment on the requested creation of CSV -> LP conversion library? I mean, there are no technical problems to do so, once you agree and provide a new influxdata git repo.

[jsteenb2]

@sranka you should be able to put the parser under the pkg directory in the monorepo here in influxdb. If we need to pull it out, we can always do so.

[sranka]

@jsteenb2 ok, "csv to line protocol conversion" is now in pkg/csv2lp

[jsteenb2]

@sranka, we'd like to see this PR broken up and submitted incrementally. First PR for the csv converter and another for the CLI. We'll have different stakeholders reviewing for the csv PR.

[sranka]

@jsteenb2 thanks for letting me know of what is required.