chore(splunk hec sink)!: json encoding should get message first and then parse #20297

pront · 2024-04-12T23:19:27Z

No description provided.

…en parse

pront · 2024-04-12T23:20:15Z

Putting this out there to potentially get some feedback, I will revisit on Monday.

datadog-vectordotdev · 2024-04-12T23:43:29Z

Datadog Report

Branch report: pront/OPA-1526_splunk_sink_enc_fix
Commit report: dbde866
Test service: vector

❌ 11 Failed (0 Known Flaky), 2146 Passed, 0 Skipped, 19m 46.3s Wall Time

❌ Failed Tests (11)

This report shows up to 5 failed tests.

sinks::humio::metrics::tests::multi_value_tags - vector
sinks::humio::metrics::tests::smoke_json - vector
sinks::splunk_hec::logs::integration_tests::splunk_auto_extracted_timestamp - vector - Details
Expand for error
```
 Test has failed
```
sinks::splunk_hec::logs::integration_tests::splunk_configure_hostname - vector - Details
Expand for error
```
 Test has failed
```
sinks::splunk_hec::logs::integration_tests::splunk_custom_fields - vector - Details
Expand for error
```
 Test has failed
```

pront · 2024-04-14T17:01:06Z

src/sinks/splunk_hec/logs/encoder.rs

+                                        .ok()?,
+                                )
+                            } else {
+                                encoder.encode(event, &mut bytes).ok()?;


🗒️ Encoders call get_message() internally.

pront · 2024-04-15T21:46:12Z

src/sinks/splunk_hec/logs/encoder.rs

-                                    .map_err(|error| emit!(SplunkEventEncodeError { error }))
-                                    .ok()?,
-                            )
+                        let hec_event = if let Some(message) = event.as_log().get_message() {


TODO: This is expected to break all integration tests using JsonSerializerConfig. For example, look at splunk_custom_fields, looking up by message is no longer correct. The code needs to be updated to directly access the value from the event.

Also, all the tests rely on the legacy namespace. I might go ahead and use the Vector namesapce.

cc @vladimir-dd and @jszwedko for confirming before I go ahead and edit all those.

I think we should move all other fields from LogEvent to HecData.fields otherwise we could lost some fields.

Sorry, didn't notice that we pass fields through metadata.fields. But since we do not expose indexed_fields for the sink, do we always pass empty.fields? Is this an expected behaviour? 🤔

I feel like I'm getting a bit turned around by this now 😵

I think I'd talked to Pavlos about only encoding the message field, but, thinking about it more now, the field in the HEC payload is actually called event which makes me think maybe it should be encoding the entire event and not just the message key. Their definition of event:

Event data can be assigned to the "event" key within the JSON object in the HTTP request, or it can be raw text. The "event" key is at the same level within the JSON event packet as the metadata keys. Within the "event" key-value curly brackets, the data can be in whatever format you want: a string, a number, another JSON object, and so on.

You can batch multiple events in one event packet by combining them within the request. By batching the events, you're specifying that any event metadata within the request is to apply to all of the events contained in the request. Batching events can significantly speed performance when you need to index large quantities of data.

https://docs.splunk.com/Documentation/Splunk/9.2.1/Data/FormateventsforHTTPEventCollector#Event_data:~:text=from%20the%20request.-,Event%20data,-Event%20data%20can

fields is defined as:

The fields key isn't applicable to raw data. This key specifies a JSON object that contains a flat (not nested) list of explicit custom fields to be defined at index time. Requests containing the "fields" property must be sent to the /collector/event endpoint, or else they aren't indexed. For more information, see Indexed field extractions.

I agree with @vladimir-dd that if we only encode message as event that maybe the rest of the fields should go into fields; however, now I'm not so sure that the entire event shouldn't just go in event.

This might be a case where we need to play around with it a bit and see how it looks like in the Splunk interface under various conditions (like Splunk HEC -> Splunk and Syslog -> Splunk).

Apologies for what feels like going in circles a bit here. I'm happy to chat about this if it helps resolve it more quickly.

Marking it as a draft for now. It's not urgent to get to the bottom of this given we have other priorities.

sebastiantia

Didn't mean to approve this PR 😓

pront · 2024-04-15T21:57:41Z

Didn't mean to approve this PR 😓

I think this will be blocked until you re-approve. I suggest re-approving 😅
I won't merge until Vlad and Jesse take a look.

vladimir-dd · 2024-04-16T09:44:24Z

src/sinks/splunk_hec/logs/encoder.rs

                        } else {
-                            encoder.encode(event, &mut bytes).ok()?;
-                            HecEvent::Text(String::from_utf8_lossy(&bytes))
+                            HecEvent::Text("".into())


I think we should use the same logic as above here to avoid losing information - but instead of encoding message encode the whole event(either as json, or as text)?

vladimir-dd

Left some comments.

pront · 2024-04-16T17:09:21Z

src/sinks/splunk_hec/logs/integration_tests.rs

@@ -525,10 +525,7 @@ async fn splunk_auto_extracted_timestamp() {
        // Splunk will determine the timestamp from the *message* field in the event.
        // Thus, we expect the `timestamp` field to still be present.
        assert_eq!(


🗒️ cargo vdev -v int start -a splunk works in Ubuntu. Also, when working on Ubuntu cargo vdev -v int test --retries 2 -a splunk fails to runs tests.

pront · 2024-04-16T17:15:00Z

src/sinks/splunk_hec/logs/integration_tests.rs

@@ -525,10 +525,7 @@ async fn splunk_auto_extracted_timestamp() {
        // Splunk will determine the timestamp from the *message* field in the event.


General comment for these tests, they all use the Legacy namespace.

jszwedko · 2024-04-16T21:46:02Z

changelog.d/splunk_hec_sink_encode_message_only.breaking.md

@@ -0,0 +1,2 @@
+The Splunk HEC sink is now using the `message` meaning to retrieve the relevant value and encodes the retrieved value
+only. Note that if the retrieved value is `None`, an event with an empty message will be published.


A few questions:

Do we know how Splunk reacts if you send it an event with an empty event field?

Is the semantic field required? I.e. will Vector fail to boot if there is no semantic message defined?

Also, is this only true for the /event endpoint and not for /raw?

jszwedko · 2024-04-16T21:58:47Z

src/sinks/splunk_hec/logs/encoder.rs

-                                    .map_err(|error| emit!(SplunkEventEncodeError { error }))
-                                    .ok()?,
-                            )
+                        let hec_event = if let Some(message) = event.as_log().get_message() {


I feel like I'm getting a bit turned around by this now 😵

I think I'd talked to Pavlos about only encoding the message field, but, thinking about it more now, the field in the HEC payload is actually called event which makes me think maybe it should be encoding the entire event and not just the message key. Their definition of event:

Event data can be assigned to the "event" key within the JSON object in the HTTP request, or it can be raw text. The "event" key is at the same level within the JSON event packet as the metadata keys. Within the "event" key-value curly brackets, the data can be in whatever format you want: a string, a number, another JSON object, and so on.

You can batch multiple events in one event packet by combining them within the request. By batching the events, you're specifying that any event metadata within the request is to apply to all of the events contained in the request. Batching events can significantly speed performance when you need to index large quantities of data.

https://docs.splunk.com/Documentation/Splunk/9.2.1/Data/FormateventsforHTTPEventCollector#Event_data:~:text=from%20the%20request.-,Event%20data,-Event%20data%20can

fields is defined as:

The fields key isn't applicable to raw data. This key specifies a JSON object that contains a flat (not nested) list of explicit custom fields to be defined at index time. Requests containing the "fields" property must be sent to the /collector/event endpoint, or else they aren't indexed. For more information, see Indexed field extractions.

I agree with @vladimir-dd that if we only encode message as event that maybe the rest of the fields should go into fields; however, now I'm not so sure that the entire event shouldn't just go in event.

This might be a case where we need to play around with it a bit and see how it looks like in the Splunk interface under various conditions (like Splunk HEC -> Splunk and Syslog -> Splunk).

Apologies for what feels like going in circles a bit here. I'm happy to chat about this if it helps resolve it more quickly.

neuronull · 2024-04-17T20:28:09Z

👋
Noting that we elected to hold off on this for the time being.
We can revisit it if needed next week when Pavlos returns.

Thanks for all the input @jszwedko 🙇

chore(splunk hec sink): json encoding should get message first and th…

bf3fcea

…en parse

pront requested a review from a team as a code owner April 12, 2024 23:19

pront requested a review from jszwedko April 12, 2024 23:19

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

pront commented Apr 14, 2024

View reviewed changes

fix some of the failing tests

7673809

sebastiantia approved these changes Apr 15, 2024

View reviewed changes

pront commented Apr 15, 2024

View reviewed changes

pront requested review from neuronull and vladimir-dd April 15, 2024 21:50

sebastiantia suggested changes Apr 15, 2024

View reviewed changes

sebastiantia self-requested a review April 15, 2024 21:52

vladimir-dd reviewed Apr 16, 2024

View reviewed changes

vladimir-dd requested changes Apr 16, 2024

View reviewed changes

sebastiantia approved these changes Apr 16, 2024

View reviewed changes

pront commented Apr 16, 2024

View reviewed changes

jszwedko changed the title ~~chore!(splunk hec sink): json encoding should get message first and then parse~~ chore(splunk hec sink)!: json encoding should get message first and then parse Apr 16, 2024

jszwedko reviewed Apr 16, 2024

View reviewed changes

pront marked this pull request as draft April 22, 2024 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(splunk hec sink)!: json encoding should get message first and then parse #20297

chore(splunk hec sink)!: json encoding should get message first and then parse #20297

pront commented Apr 12, 2024

pront commented Apr 12, 2024

datadog-vectordotdev bot commented Apr 12, 2024 •

edited

pront Apr 14, 2024 •

edited

pront Apr 15, 2024 •

edited

pront Apr 15, 2024

vladimir-dd Apr 16, 2024

vladimir-dd Apr 16, 2024 •

edited

jszwedko Apr 16, 2024

pront Apr 22, 2024

sebastiantia left a comment

pront commented Apr 15, 2024

vladimir-dd Apr 16, 2024 •

edited

vladimir-dd left a comment

pront Apr 16, 2024

pront Apr 16, 2024

jszwedko Apr 16, 2024

jszwedko Apr 16, 2024

neuronull commented Apr 17, 2024

		@@ -525,10 +525,7 @@ async fn splunk_auto_extracted_timestamp() {
		// Splunk will determine the timestamp from the message field in the event.

		@@ -0,0 +1,2 @@
		The Splunk HEC sink is now using the `message` meaning to retrieve the relevant value and encodes the retrieved value
		only. Note that if the retrieved value is `None`, an event with an empty message will be published.

chore(splunk hec sink)!: json encoding should get message first and then parse #20297

Are you sure you want to change the base?

chore(splunk hec sink)!: json encoding should get message first and then parse #20297

Conversation

pront commented Apr 12, 2024

pront commented Apr 12, 2024

datadog-vectordotdev bot commented Apr 12, 2024 • edited

Datadog Report

❌ Failed Tests (11)

pront Apr 14, 2024 • edited

Choose a reason for hiding this comment

pront Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-dd Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastiantia left a comment

Choose a reason for hiding this comment

pront commented Apr 15, 2024

vladimir-dd Apr 16, 2024 • edited

Choose a reason for hiding this comment

vladimir-dd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neuronull commented Apr 17, 2024

datadog-vectordotdev bot commented Apr 12, 2024 •

edited

pront Apr 14, 2024 •

edited

pront Apr 15, 2024 •

edited

vladimir-dd Apr 16, 2024 •

edited

vladimir-dd Apr 16, 2024 •

edited