fix(loki sinks): Fix loki event timestamp out of range panic #20780

suikammd · 2024-07-03T03:27:27Z

Fix #20763

jorgehermo9 · 2024-07-03T11:04:34Z

src/sinks/loki/tests.rs

+    let mut sink = LokiSink::new(config, client).unwrap();
+
+    let mut e1 = LogEvent::from("hello world");
+    use vector_lib::config::log_schema;


should we move this use statement to the beginning of this tests module?

jszwedko

Thanks @suikammd ! I left a question below about the behavior.

jszwedko · 2024-07-03T18:28:58Z

src/internal_events/loki.rs

+
+impl InternalEvent for LokiEventTimestampOutOfRangeError {
+    fn emit(self) {
+        error!(


Should we make this a warn! since it doesn't actually inhibit processing? Also, what happens when we send an event to Loki without a timestamp? Does it just use the current timestamp?

According to Loki api, timestamp is must.

Oh I see, I missed that you are returning early. In that case we'd want to emit the metrics for errors and discarded events. See an example here:

vector/src/internal_events/splunk_hec.rs

Lines 55 to 77 in 8325300

impl<'a> InternalEvent for SplunkInvalidMetricReceivedError<'a> {

fn emit(self) {

error!(

message = "Invalid metric received.",

error = ?self.error,

error_type = error_type::INVALID_METRIC,

stage = error_stage::PROCESSING,

value = ?self.value,

kind = ?self.kind,

internal_log_rate_limit = true,

);

counter!(

"component_errors_total", 1,

"error_type" => error_type::INVALID_METRIC,

"stage" => error_stage::PROCESSING,

);

counter!(

"component_discarded_events_total", 1,

"error_type" => error_type::INVALID_METRIC,

"stage" => error_stage::PROCESSING,

);

}

}

jorgehermo9 · 2024-07-05T09:43:47Z

src/sinks/loki/tests.rs

+
+#[tokio::test]
+async fn timestamp_out_of_range() {
+    use vector_lib::config::log_schema;


I meant the beggining of this module, about line 10. It is more idiomatic to have all the use statements at the begginning of the module, so we don't have to dive into each scope and find those statements.

What do you think?

Get it, check the new commit.

yes, thats it! Thanks :)

jszwedko · 2024-07-08T19:54:42Z

src/sinks/loki/sink.rs

+                Some(timestamp) => timestamp,
+                None => {
+                    emit!(LokiEventTimestampOutOfRangeError);
+                    return None;


I think we also need to use the finalizers here and explicitly nack the event via update_status like finalizers.update_status(EventStatus::Errored). Otherwise Vector will consider them to have been successfully processed.

jszwedko · 2024-07-09T14:00:12Z

src/sinks/loki/sink.rs

+            Some(Value::Timestamp(ts)) => match ts.timestamp_nanos_opt() {
+                Some(timestamp) => timestamp,
+                None => {
+                    let finalizers = event.take_finalizers();


I think you already have the finalizers separated above on line 250.

sorry, I missed this line

jszwedko

Thanks @suikammd !

suikammd · 2024-07-12T07:16:53Z

@jszwedko It seems like I don't have the permissions to add the no-changelog label, can you help add this label?

jszwedko · 2024-07-12T14:17:39Z

Thanks for the bump @suikammd . This change should actually have a changelog entry. Do you mind adding one? See https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md for details.

suikammd · 2024-07-12T16:41:31Z

Thanks for the bump @suikammd . This change should actually have a changelog entry. Do you mind adding one? See https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md for details.

I add a changelog, but I accidentally rebased the master branch :(, sorry for that.

jszwedko

I think you still need to fill in the changelog fragment :) See https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md for details about what the content should look like.

jorgehermo9 · 2024-07-13T11:19:05Z

changelog.d/20763_loki_sink_drop_invalid_timestamp_event.fix.md

@@ -0,0 +1,3 @@
+Loki sink drop events with timestamp can not parsed by chrono.


What about something like this?

Suggested change

Loki sink drop events with timestamp can not parsed by chrono.

Loki sink now drops events with non-parseable timestamps.

Get it, but the word parseable is not recognized. Maybe should update the checklist.

i'm not an english native speaker, maybe @jszwedko can help us here😁. I think the spelling is correct

jszwedko · 2024-07-15T20:02:28Z

changelog.d/20763_loki_sink_drop_invalid_timestamp_event.fix.md

@@ -0,0 +1,3 @@
+Loki sink now drops events with non-processable timestamps.


Suggested change

Loki sink now drops events with non-processable timestamps.

Loki sink now drops events with non-parsable timestamps.

I think parsable is actually the correct spelling, but what you have here seems fine to me too. Thanks for adding the changelog!

github-actions · 2024-07-17T14:20:49Z

Regression Detector Results

Run ID: f0915b07-01f9-49db-8378-34607e979f24 Metrics dashboard

Baseline: 5a10aa2
Comparison: 6d179e5

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI	links
❌	file_to_blackhole	egress throughput	-9.48	[-16.31, -2.64]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	http_elasticsearch	ingress throughput	+2.83	[+2.60, +3.07]
➖	datadog_agent_remap_datadog_logs	ingress throughput	+2.75	[+2.55, +2.94]
➖	http_to_http_acks	ingress throughput	+1.38	[+0.04, +2.72]
➖	fluent_elasticsearch	ingress throughput	+1.12	[+0.62, +1.62]
➖	syslog_log2metric_splunk_hec_metrics	ingress throughput	+0.70	[+0.59, +0.81]
➖	syslog_humio_logs	ingress throughput	+0.69	[+0.56, +0.83]
➖	otlp_http_to_blackhole	ingress throughput	+0.69	[+0.51, +0.87]
➖	syslog_regex_logs2metric_ddmetrics	ingress throughput	+0.44	[+0.31, +0.56]
➖	datadog_agent_remap_datadog_logs_acks	ingress throughput	+0.36	[+0.16, +0.56]
➖	datadog_agent_remap_blackhole	ingress throughput	+0.26	[+0.16, +0.37]
➖	socket_to_socket_blackhole	ingress throughput	+0.15	[+0.08, +0.21]
➖	http_text_to_http_json	ingress throughput	+0.12	[-0.00, +0.25]
➖	splunk_hec_indexer_ack_blackhole	ingress throughput	+0.02	[-0.07, +0.10]
➖	http_to_s3	ingress throughput	+0.01	[-0.25, +0.28]
➖	http_to_http_noack	ingress throughput	+0.01	[-0.12, +0.14]
➖	splunk_hec_route_s3	ingress throughput	+0.01	[-0.32, +0.34]
➖	splunk_hec_to_splunk_hec_logs_acks	ingress throughput	-0.00	[-0.13, +0.12]
➖	splunk_hec_to_splunk_hec_logs_noack	ingress throughput	-0.01	[-0.10, +0.09]
➖	http_to_http_json	ingress throughput	-0.14	[-0.22, -0.05]
➖	syslog_loki	ingress throughput	-0.42	[-0.51, -0.33]
➖	datadog_agent_remap_blackhole_acks	ingress throughput	-0.51	[-0.65, -0.37]
➖	otlp_grpc_to_blackhole	ingress throughput	-0.93	[-1.06, -0.81]
➖	syslog_splunk_hec_logs	ingress throughput	-1.02	[-1.12, -0.92]
➖	syslog_log2metric_tag_cardinality_limit_blackhole	ingress throughput	-1.63	[-1.72, -1.53]
➖	syslog_log2metric_humio_metrics	ingress throughput	-2.80	[-2.95, -2.65]
❌	file_to_blackhole	egress throughput	-9.48	[-16.31, -2.64]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

…otdev#20780) * fix(loki sinks): Fix loki event timestamp out of range panic * change LokiEventTimestampOutOfRangeError error to warn * move use to begin of file * update skipped events finalizer to errored * fix: remove duplicate take finalizers * add changelog * fix changelog name * fix spelling issue * modify changelog contents * modify changelog contents * fix by check-events script error * fix word * fix word * fix word * fix cargo fmt

suikammd requested a review from a team as a code owner July 3, 2024 03:27

github-actions bot added the domain: sinks Anything related to the Vector's sinks label Jul 3, 2024

jorgehermo9 reviewed Jul 3, 2024

View reviewed changes

jszwedko reviewed Jul 3, 2024

View reviewed changes

jorgehermo9 reviewed Jul 5, 2024

View reviewed changes

jszwedko reviewed Jul 8, 2024

View reviewed changes

jszwedko reviewed Jul 9, 2024

View reviewed changes

jszwedko approved these changes Jul 9, 2024

View reviewed changes

jszwedko enabled auto-merge July 9, 2024 15:48

suikammd added 6 commits July 13, 2024 00:24

fix(loki sinks): Fix loki event timestamp out of range panic

3597c1e

change LokiEventTimestampOutOfRangeError error to warn

06dcc38

move use to begin of file

6ee937e

update skipped events finalizer to errored

f0c689d

fix: remove duplicate take finalizers

54bcfba

add changelog

c395af6

auto-merge was automatically disabled July 12, 2024 16:39
Head branch was pushed to by a user without write access

suikammd force-pushed the master branch from ef3d173 to c395af6 Compare July 12, 2024 16:39

jszwedko reviewed Jul 12, 2024

View reviewed changes

fix changelog name

ee29fbc

suikammd force-pushed the master branch from 483ee98 to ee29fbc Compare July 12, 2024 16:52

fix spelling issue

7845582

jorgehermo9 reviewed Jul 13, 2024

View reviewed changes

suikammd added 2 commits July 13, 2024 16:33

modify changelog contents

e7d7b5e

modify changelog contents

46150b4

jszwedko approved these changes Jul 15, 2024

View reviewed changes

jszwedko enabled auto-merge July 15, 2024 20:02

fix by check-events script error

64ce9c1

auto-merge was automatically disabled July 16, 2024 08:34
Head branch was pushed to by a user without write access

suikammd added 3 commits July 16, 2024 16:43

fix word

0a81131

fix word

5147f41

fix word

987b0d8

jszwedko approved these changes Jul 16, 2024

View reviewed changes

jszwedko enabled auto-merge July 16, 2024 13:02

fix cargo fmt

2b625ca

auto-merge was automatically disabled July 17, 2024 03:36
Head branch was pushed to by a user without write access

jszwedko enabled auto-merge July 17, 2024 12:26

jszwedko added this pull request to the merge queue Jul 17, 2024

Merged via the queue into vectordotdev:master with commit 6d179e5 Jul 17, 2024

vparfonov mentioned this pull request Jul 24, 2025

LOG-7417: Fix loki event timestamp out of range panic ViaQ/vector#217

Merged

	impl<'a> InternalEvent for SplunkInvalidMetricReceivedError<'a> {
	fn emit(self) {
	error!(
	message = "Invalid metric received.",
	error = ?self.error,
	error_type = error_type::INVALID_METRIC,
	stage = error_stage::PROCESSING,
	value = ?self.value,
	kind = ?self.kind,
	internal_log_rate_limit = true,
	);
	counter!(
	"component_errors_total", 1,
	"error_type" => error_type::INVALID_METRIC,
	"stage" => error_stage::PROCESSING,
	);
	counter!(
	"component_discarded_events_total", 1,
	"error_type" => error_type::INVALID_METRIC,
	"stage" => error_stage::PROCESSING,
	);
	}
	}

		@@ -0,0 +1,3 @@
		Loki sink drop events with timestamp can not parsed by chrono.

	Loki sink drop events with timestamp can not parsed by chrono.
	Loki sink now drops events with non-parseable timestamps.

		@@ -0,0 +1,3 @@
		Loki sink now drops events with non-processable timestamps.

fix(loki sinks): Fix loki event timestamp out of range panic #20780

fix(loki sinks): Fix loki event timestamp out of range panic #20780

Uh oh!

Conversation

suikammd commented Jul 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jszwedko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jszwedko left a comment

Choose a reason for hiding this comment

Uh oh!

suikammd commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jszwedko commented Jul 12, 2024

Uh oh!

suikammd commented Jul 12, 2024

Uh oh!

jszwedko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgehermo9 Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 17, 2024

Regression Detector Results

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

suikammd commented Jul 12, 2024 •

edited

Loading

jorgehermo9 Jul 13, 2024 •

edited

Loading