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

[network_traffic] Set map_to_ecs to true by default #10785

Merged

Conversation

fearful-symmetry
Copy link
Contributor

Proposed commit message

See #8185

This sets map_to_ecs for all the network_traffic datastreams to true, in line with #8185.

I don't have a huge amount of context for this, so apologies if I forgot something.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

@fearful-symmetry fearful-symmetry added the Team:Security-Linux Platform Linux Platform Security team [elastic/sec-linux-platform] label Aug 13, 2024
@fearful-symmetry fearful-symmetry requested a review from efd6 August 13, 2024 21:16
@fearful-symmetry fearful-symmetry self-assigned this Aug 13, 2024
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner August 13, 2024 21:16
@elasticmachine
Copy link

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

@andrewkroh andrewkroh added the Integration:network_traffic Network Packet Capture label Aug 13, 2024
@fearful-symmetry
Copy link
Contributor Author

So, @efd6 I kind of want you to check my logic here. It looks like CI is failing on a handful of mappings:

test case failed: one or more errors found in documents stored in logs-network_traffic.mongodb-12810 data stream: [0] parsing field value failed: field "network_traffic.mongodb.cursorId" value "0" (float64): expected string or array of strings
[1] parsing field value failed: field "network_traffic.mongodb.startingFrom" value "0" (float64): expected string or array of strings

Looking at the packetbeat code, it looks like cursorId and startingFrom are supposed to be integers:

	m.event["cursorId"], err = d.readInt64()
	if err != nil {
		logp.Err("An error occurred while parsing OP_REPLY message: %s", err)
		return false, false
	}
	m.event["startingFrom"], err = d.readInt32()
	if err != nil {
		logp.Err("An error occurred while parsing OP_REPLY message: %s", err)
		return false, false
	}

However, I'm not sure why this wasn't caught earlier, and I'm also not sure if I'm just...allowed to change the types of ECS fields?

@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@andrewkroh andrewkroh changed the title Set map_to_ecs to true by default [network_traffic] Set map_to_ecs to true by default Aug 21, 2024
@andrewkroh andrewkroh added the enhancement New feature or request label Aug 21, 2024
@@ -27,7 +27,7 @@
The number of documents in the reply.

- name: startingFrom
type: keyword
type: long
Copy link
Member

@andrewkroh andrewkroh Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should change any of these from keyword to long. I think they are all keywords in standalone Packetbeat mappings too, right? Unless we know that range queries1 need to be supported, then I would keep them as is. To make the tests pass there are two solutions:

  1. Convert numbers to strings via processors.
  2. Add numeric_keyword_fields to the test configuration. (see https://github.com/elastic/elastic-package/blob/main/docs/howto/pipeline_testing.md)

Footnotes

  1. https://www.elastic.co/guide/en/elasticsearch/reference/current/number.html#_which_type_should_i_use

Copy link
Contributor Author

@fearful-symmetry fearful-symmetry Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, annoyingly, the packetbeat fields don't seem to have types set?

        - name: startingFrom
          description: >
            Where in the cursor this reply is starting.
        - name: cursorId
          description: >
            The cursor identifier returned in the OP_REPLY. This must be the value that was returned from the database.

But the other fields do have the correct type set:

        - name: numberToReturn
          type: long
          description: >
            The requested maximum number of documents to be returned.

We do read them in as ints though:

	m.event["startingFrom"], err = d.readInt32()
	if err != nil {
		logp.Err("An error occurred while parsing OP_REPLY message: %s", err)
		return false, false
	}

I wonder if that's how these got set to the wrong type to begin with? Either way, it seems like we won't be conflicting with any existing mapping? Maybe? Unless they default to keywords?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, annoyingly, the packetbeat fields don't seem to have types set?

The default is keyword in Beats fields.yml framework. So these fields are keywords in Beats.

I wonder if that's how these got set to the wrong type to begin with?

I don't think cursorId is wrong. A keyword is generally the best choice for identifiers when numeric range queries are not needed.

There might be some use to having startingFrom be indexed as a long, but I wouldn't change from keyword to long without good reason like a feature request. These fields are associated with mongodb opcodes that were deprecated in mongodb 5.0 from July 2021 (and that is now two majors ago), so doubt they are getting used much anyways.

I think adding numeric_keyword_fields: [...] to the test configuration to remove the strict check that _source contains strings is the best course of action to resolve the test failure.

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @fearful-symmetry

@fearful-symmetry fearful-symmetry merged commit 06fde1e into elastic:main Aug 22, 2024
5 checks passed
@elasticmachine
Copy link

Package network_traffic - 1.32.0 containing this change is available at https://epr.elastic.co/search?package=network_traffic

@efd6
Copy link
Contributor

efd6 commented Sep 2, 2024

It looks to me like the sample events were not updated here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:network_traffic Network Packet Capture Team:Security-Linux Platform Linux Platform Security team [elastic/sec-linux-platform]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants