fix(parsers.avro): Do not force addition of timestamp as a field

[athornton]

Required for all PRs

• [ n/a ] Updated associated README.md.
• [ X ] Wrote appropriate unit tests.
• [ X ] Pull request title or commits are in conventional commit format

resolves #13821

Dropped the forced addition of the timestamp as a field, and touched up the JSON test case to note that I did want the timestamp as a field.

Note that this also includes the fix for Issue #13854 / PR #13855, because it impacts correct timestamp specification. We might want to just merge this PR instead and drop that one, although it's just going to be "skipping already-applied-commit" if we don't.

[athornton]

Ah, I see the problem.

The field selection is done before the parsing-as-timestamp, so if you tell the plugin you want as a timestamp, and then it's not one of your fields: could not parse \"testdata/json-array/message.json\": could not parse '<nil>' to 'unix_ms' because the field has already been dropped.

So all I need to do there is take the timestamp from the raw input data, and not from the stuff already parsed into fields.

[athornton]

There's another case here, and I think what I am doing is the right thing but I do not know.

What I have is working fine for flattening or not flattening maps.

But even if you are not flattening the map (which is to say, you don't get measurements with the data type stitched to the field), I'm still flattening arrays, to {field_name}{field_sep}{index}, because I don't see a better way to do array values that will work with InfluxDB line protocol.

Example: you have data = [ 0, 1, 2, 3 ] and avro_field_separator = "_" ; this goes into your measurement as data_0=0,data_1=1,data_2=2,data_3=3

(The only other remotely plausible way seems like it would be outputting a string which is the JSON representation of the array, but that requires knowledge of how to interpret that string on the receiving end.)

And while it's true that people may not be outputting to InfluxDB, the telegraf testutils definitely expect to be able to parse metrics from a file that's in line protocol, so I think it's safe to consider that our default format.

[srebhan]

@athornton before giving this a review I'm trying to understand why the behavior was what it was... Please bear with me as I'm not an Avro expert. Some questions:

1. Is my assumption correct that you always get a timestamp in an Avro message?
2. I read the code as, we output the timestamp as a field only if you set the timestamp option?
3. Is there another way to extract the timestamp field from the Avro message with the existing means in the parser?

[athornton]

I'm not an Avro expert either, but I can ask @afausti about, at least, how we're doing it here.

1. I think that Avro messages do not necessarily have a timestamp. But you are sending it to Telegraf and putting it into InfluxDB line protocol, which necessarily DOES have a timestamp. So if there's not a field in that message you can identify as a timestamp, you should just use the time you handled the message as the timestamp. At least, I don't see anything in https://avro.apache.org/docs/1.11.1/specification/ that says to me that you must have a timestamp.

2. The prior behavior was that, if we did identify a field in the incoming Avro message as a timestamp, that would always be added to our InfluxDB fields, even if we were explicitly listing the things we wanted as fields and that was not one of them. This is what the issue was about: since line protocol always has a timestamp, it's not necessary for it to be a separate field. Although in my use case we always do want it as a field, this turns out to not be generally true.

3. Not that I can think of. Avro contains some logical types (basically type aliases) for date and time encoding (https://avro.apache.org/docs/1.11.1/specification/#timestamp-millisecond-precision) but they boil down to a long for number-of-millis-or-micros-since-the-Unix-epoch. We could attempt autodetection of the timestamp types, but that seems very fragile. I know in our own use case that by the time a message gets to telegraf it has several fields of that type, as each stage of the processing pipeline stamps its own "I handled this message at" field into it, and Rubin is probably not unique in this regard. Just saying what the timestamp field is will be simpler and less fragile, I think.

And actually I have a question about #1: The test utils all assume that you're basically parsing to line protocol. Is that a good generic assumption about telegraf? Obviously there are at least some people who do not output to InfluxDB at all, but in my mind, the point of telegraf is primarily to feed InfluxDB and other output cases are secondary--that is, I should think of it as a very flexible converter for InfluxDB measurements, and not as a generic converter like some kind of imagemagick equivalent for metric events. But I don't know if I'm actually correct about that.

[srebhan]

@athornton can you please rebase this after PR #13855 is merged!? Furthermore, I do have one question regarding the supplied_timestamp_fields_unspecified test case...

[srebhan]

Looks good to me. Thanks @athornton!