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

[receiver/netflow] Add the netflow receiver - PR 1 #34164

Merged
merged 41 commits into from
Nov 25, 2024

Conversation

dlopes7
Copy link
Contributor

@dlopes7 dlopes7 commented Jul 19, 2024

This adds the base implementation of config and factory for the netflow receiver, according to CONTRIBUTING.md.
Later another PR will be opened with the actual implementation of the receiver.

It was not clear to me if the CHANGELOG entry should be added on this PR, or the one that actually implements the receiver, I am choosing to add it to the second PR.

Link to tracking Issue: #32732

Testing: Added tests for the config, receiver and listener creation

Documentation: Added the first version of the README with sample yaml and a documentation of the different fields.

@dlopes7 dlopes7 requested review from a team and atoulme July 19, 2024 02:45
Copy link

linux-foundation-easycla bot commented Jul 19, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@songy23 songy23 requested a review from evan-bradley July 19, 2024 13:18
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Thanks @dlopes7.

Could you run make chlog-new and fill out a new_component changelog?

receiver/netflowreceiver/testdata/config.yaml Outdated Show resolved Hide resolved
receivers:
netflow:
listeners:
- scheme: netflow
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I configure multiple netflow listeners, or is it one listener per scheme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but now that you mention it I think it might make more sense to allow a single listener per netflow config

Copy link

Choose a reason for hiding this comment

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

Consider supporting the auto-detect feature of goflow2 vs making users explicitly set what flow type they are using.

receiver/netflowreceiver/factory.go Outdated Show resolved Hide resolved
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Just a couple nits, hopefully it's helpful 👍

receiver/netflowreceiver/README.md Outdated Show resolved Hide resolved
receiver/netflowreceiver/README.md Show resolved Hide resolved
receiver/netflowreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/netflowreceiver/README.md Outdated Show resolved Hide resolved
dlopes7 and others added 6 commits August 9, 2024 18:57
* Improve readme, showing default values
* Allow a single listener per config
* Remove the contrib distribution from the metadata
* Add the changelog entry
@LucasHocker
Copy link

@dlopes7 are we awaiting further reviews or code changes from the reviews?

@dlopes7
Copy link
Contributor Author

dlopes7 commented Oct 30, 2024

Based on the proposed format changes I am suggesting this format for the output:

type NetworkAddress struct {
	Address []byte `json:"address,omitempty"`
	Port    uint32 `json:"port,omitempty"`
}

type Flow struct {
	Type           string `json:"type,omitempty"`
	TimeReceived   uint64 `json:"time_received,omitempty"`
	Start          uint64 `json:"start,omitempty"`
	End            uint64 `json:"end,omitempty"`
	SequenceNum    uint32 `json:"sequence_num,omitempty"`
	SamplingRate   uint64 `json:"sampling_rate,omitempty"`
	SamplerAddress []byte `json:"sampler_address,omitempty"`
}

type Protocol struct {
	Name []byte `json:"name,omitempty"` // Layer 7
}

type NetworkIO struct {
	Bytes   uint64 `json:"bytes,omitempty"`
	Packets uint64 `json:"packets,omitempty"`
}

type OtelNetworkMessage struct {
	Source      NetworkAddress `json:"source,omitempty"`
	Destination NetworkAddress `json:"destination,omitempty"`
	Transport   string         `json:"transport,omitempty"` // Layer 4
	Type        string         `json:"type,omitempty"`      // Layer 3
	IO          NetworkIO      `json:"io,omitempty"`
	Flow        Flow           `json:"flow,omitempty"`
}

@evan-bradley
Copy link
Contributor

@dlopes7 Can you rebase on main and add your GitHub handle to cmd/githubgen/allowlist.txt?

@dlopes7
Copy link
Contributor Author

dlopes7 commented Nov 8, 2024

New format after changes following semantic conventions would be

{
    "destination": {
        "address": "192.168.0.1",
        "port": 22
    },
    "flow": {
        "end": 1731073104662487000,
        "sampler_address": "192.168.0.2",
        "sequence_num": 49,
        "start": 1731073077662487000,
        "time_received": 1731073138662487000,
        "type": "NETFLOW_V5"
    },
    "io": {
        "bytes": 529,
        "packets": 378
    },
    "source": {
        "address": "192.168.0.3",
        "port": 40
    },
    "transport": "TCP",
    "type": "IPv4"
}

@dlopes7 dlopes7 requested a review from a team as a code owner November 8, 2024 13:46
@evan-bradley
Copy link
Contributor

@dlopes7 It looks like there are some conflicts with main, can you pull in the latest from main into your branch and resolve the conflicts?

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Thanks @dlopes7. To reduce churn on common files (issue templates, codeowners, etc.) I'm going to merge this and we can address open conversations in the follow-up PR.

@evan-bradley evan-bradley merged commit 5698c9d into open-telemetry:main Nov 25, 2024
158 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 25, 2024
shivanthzen pushed a commit to shivanthzen/opentelemetry-collector-contrib that referenced this pull request Dec 5, 2024
ZenoCC-Peng pushed a commit to ZenoCC-Peng/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2024
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants