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

Processors.parser and data_format="prometheus" and timestamps #16331

Open
knollet opened this issue Dec 18, 2024 · 0 comments
Open

Processors.parser and data_format="prometheus" and timestamps #16331

knollet opened this issue Dec 18, 2024 · 0 comments
Labels
bug unexpected problem or unintended behavior

Comments

@knollet
Copy link
Contributor

knollet commented Dec 18, 2024

ok, this one is a little bit complicated ;)

(I show it here with data_format="influx", because it's easier to demonstrate. But we stumbled across this problem with data_format="prometheus". The problem is with both)

Relevant telegraf.conf

###############################
## testing with influx-input

## our one testmetric is this
## testmetric,outer=true inner="testmetric,inner=true data=123" 1734500000000000000
## the inner has NO timestamp!

[[inputs.file]]
files = ["testmetric.influx"]
data_format = "influx"

[[processors.parser]]
drop_original = false
merge = "override-with-timestamp"
parse_fields = ["inner"]
data_format = "influx"        # <-- this is, what this is about

Logs from Telegraf

$ telegraf --config ex.conf --test --test-wait 5

> testmetric,host=myhost,inner=true,outer=true data=123,inner="testmetric,inner=true data=123" 1734539180486106897

System info

telegraf 1.32.1

Steps to reproduce

  1. have a processors.parser with merge="override-with-timestamp" and data to parse in either influx-format or prometheus-format but without a timestamp (inner has no timestamp!)
  2. Docs of processors.parser and merge-with-timestamp state that the parsed timestamp is used when present
  3. See time.now() being merged in even though there's no timestamp present in the input.

Expected behavior

Retain the old timestamp if none is found in the parsed data, as the docs state.

Actual behavior

Uses the (current) time of parsing.

Additional info

This seems to be caused by the parsers (influx and prometheus) setting a "current-time" timestamp in the metrics they create. But processors.parser checks for
https://github.com/influxdata/telegraf/blob/master/plugins/processors/parser/parser.go#L173
if !metric.Time().IsZero() {

(sidenote: I tried setting the inner-timestamp to 0, and THEN processors.parser DID override with 0, so 0 is not .IsZero(), ohkay...)

So, the parser has to signal with something saying yes to .IsZero() that there was no timestamp present.

But the prometheus parser always takes time of parsing:
https://github.com/influxdata/telegraf/blob/master/plugins/parsers/prometheus/metric_v1.go#L27
https://github.com/influxdata/telegraf/blob/master/plugins/parsers/prometheus/metric_v2.go#L26

Time handling in the influx-parser is more complicated, so I can't just give one line, but still: There's no docs on how to have a timestamp being .IsZero() when it is missing in the input.


Why is this relevant?

We tried to input prometheus via win_eventlog. We get an okay timestamp from the win_eventlog, and would override it with an inline timestamp from prometheus if present.
But this is not possible, even though the docs say the original timestamp is not touched if the parsed data doesn't have one.


I know that influxdb defaults to "now" if you omit the timestamp, so it makes sense to have the parser emit the time of parsing. Prometheus is probably the same. But as processors.parser reacts to the timestamp being .IsZero(), I think there should be a possibility to trigger and take advantage of that. Perhaps the parsers need a config option to toggle this.

The alternative would to have the parsers to always emit timestamps which are .IsZero() and have the inputs.file and inputs.win_eventlog and all the other inputs which use parsers to fill in the Now() for every .IsZero()


I think this problem only arises with processors.parser and not with inputs which use parsers (inputs.file or inputs.win_eventlog) alone.

The combination is what creates the problem:
If you only have an input+parser you can choose between "now" and the inline-timestamp.

But with input+parser + processors.parser you have a "now" and two possible inline-timestamps (the inline-ts of the input and the inline-ts of processors.parser), so three timestamps to choose from. And you can't correctly choose which one you want to use, even though the docs suggest that.

@knollet knollet added the bug unexpected problem or unintended behavior label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

1 participant