Migrate Netflow code to the logp logging library #42270

jrmolin · 2025-01-08T21:35:36Z

Proposed commit message

[Netflow] Migrate from log to the logp logging library

The rest of beats is using github.com/elastic/elastic-agent-libs/logp, so make Netflow use it, too.
Cleaned up the test code as well

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

[ ]

How to test this PR locally

Related issues

Implementation Detail: Logging should use logp everywhere #42247

Use cases

Screenshots

Logs

elasticmachine · 2025-01-14T17:26:57Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

elasticmachine · 2025-01-14T17:26:58Z

Pinging @elastic/sec-deployment-and-devices (Team:Security-Deployment and Devices)

andrewkroh · 2025-01-15T21:19:54Z

x-pack/filebeat/input/netflow/decoder/config/config.go

@@ -30,7 +30,7 @@ type Config struct {

 var defaultCfg = Config{
 	protocols:       []string{},
-	logOutput:       io.Discard,
+	logOutput:       logp.L().Named("netflow"),


The godoc for Defaults() says "log output is discarded".

With this line, it is hard to guarantee that is true because initialization order can affect logp.L(). The global logger may or may not be initialized at the time at which this call is executed.

I didn't look at how this is used, but maybe it's better to separate the logger from the decoder config concept. And require that users of the decoders pass their own parent *logp.Logger at construction time.

stefans-elastic · 2025-01-29T17:13:45Z

just curious - why is this PR to branch 8.x and not main?

jrmolin · 2025-01-31T18:42:04Z

I was trying to get this into 8.18, @stefans-elastic

jrmolin · 2025-01-31T18:42:23Z

what should I do now?

andrewkroh

It looks like the code doesn't build successfully at the moment.

This PR should target the main branch, then automation will open a PR to backport this into the 8.x branch if you have applied the backport-8.x label.

andrewkroh · 2025-01-31T18:45:41Z

x-pack/filebeat/input/netflow/decoder/config/config.go

-	sharedTemplates: false,
-	withCache:       false,
-}
-
 // Defaults returns a configuration object with defaults settings:
 // - no protocols are enabled.
 // - log output is discarded


This statement needs updated given the changes.

andrewkroh · 2025-01-31T19:34:18Z

x-pack/filebeat/input/netflow/decoder/v9/session_test.go

 	"github.com/stretchr/testify/assert"

 	"github.com/elastic/beats/v7/x-pack/filebeat/input/netflow/decoder/template"
 	"github.com/elastic/beats/v7/x-pack/filebeat/input/netflow/decoder/test"
 )

-var logger = log.New(io.Discard, "", 0)
+var logerr = logp.DevelopmentSetup(logp.ToDiscardOutput())


This logerr is never used. It looks like you want to initialize the logger globally. For this, you could do

func init() { logp.TestingSetup() }

or call that in the specific test functions where you expect the logging facility to be setup.

andrewkroh · 2025-01-31T19:35:15Z

x-pack/filebeat/input/netflow/decoder/v9/session_test.go

 	"github.com/stretchr/testify/assert"

 	"github.com/elastic/beats/v7/x-pack/filebeat/input/netflow/decoder/template"
 	"github.com/elastic/beats/v7/x-pack/filebeat/input/netflow/decoder/test"
 )

-var logger = log.New(io.Discard, "", 0)
+var logerr = logp.DevelopmentSetup(logp.ToDiscardOutput())
+var glogger = logp.NewLogger("")


This glogger doesn't need to exist. Any place in this file that uses it can be replaced either logp.NewLogger(name) or just logp.L().

andrewkroh · 2025-01-31T19:37:37Z

x-pack/filebeat/input/netflow/decoder/config/config.go

@@ -53,7 +51,7 @@ func (c *Config) WithProtocols(protos ...string) *Config {
 }

 // WithLogOutput sets the output io.Writer for logging.
-func (c *Config) WithLogOutput(output io.Writer) *Config {
+func (c *Config) WithLogOutput(output *logp.Logger) *Config {


I think there should only be one way to set the logger. Currently you can set it at construction through Defaults(logger), but you can also call this afterwards. I think this should be removed now that you changed Defaults().

andrewkroh · 2025-01-31T19:52:26Z

x-pack/filebeat/input/netflow/decoder/v1/v1.go

 }

 func New(config config.Config) protocol.Protocol {
-	return NewProtocol(ProtocolID, &templateV1, readV1Header, log.New(config.LogOutput(), LogPrefix, 0))
+	return NewProtocol(ProtocolID, &templateV1, readV1Header, logp.L().Named(LogPrefix))


This should utilize the "parent" logger as in config.LogOutput().Named(ProtocolName).

(this applies to other protocols as well)

andrewkroh · 2025-01-31T19:56:28Z

x-pack/filebeat/input/netflow/decoder/v1/v1.go

 	flowTemplate *template.Template
 	version      uint16
 	readHeader   ReadHeaderFn
 }

 func init() {
-	protocol.Registry.Register(ProtocolName, New)
+	_ := protocol.Registry.Register(ProtocolName, New)


What's the goal with changing this? Appease the linter? Personally, I would wrap this in panic() because if we accidentally registered two of the same protocols, then we made a big mistake, and this should fail hard during an integration test.

(this applies to other protocols as well)

yes, to appease the linter. I'll make all these edits.

migrate from log to logp

26bb98f

jrmolin added cleanup Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Security-Deployment and Devices Deployment and Devices Team in Security Solution labels Jan 8, 2025

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jan 8, 2025

mergify bot assigned jrmolin Jan 8, 2025

jrmolin mentioned this pull request Jan 8, 2025

Implementation Detail: Logging should use logp everywhere #42247

Open

Merge branch '8.x' of github.com:jrmolin/beats into 8.x-netflow-use-logp

75f21f8

jrmolin marked this pull request as ready for review January 14, 2025 17:26

jrmolin requested a review from a team as a code owner January 14, 2025 17:26

passing tests locally now

098c6bb

andrewkroh reviewed Jan 15, 2025

View reviewed changes

jrmolin added 3 commits January 23, 2025 08:52

Merge branch '8.x' of github.com:jrmolin/beats into 8.x-netflow-use-logp

6a9edf6

code review comments addressed, linting addressed

ffd42c2

Merge branch '8.x' of github.com:jrmolin/beats into 8.x-netflow-use-logp

7106b06

jrmolin requested a review from andrewkroh January 29, 2025 15:03

andrewkroh reviewed Jan 31, 2025

View reviewed changes

code review updates

e2cf24a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Netflow code to the logp logging library #42270

Migrate Netflow code to the logp logging library #42270

jrmolin commented Jan 8, 2025 •

edited

Loading

elasticmachine commented Jan 14, 2025

elasticmachine commented Jan 14, 2025

andrewkroh Jan 15, 2025

stefans-elastic commented Jan 29, 2025

jrmolin commented Jan 31, 2025

jrmolin commented Jan 31, 2025

andrewkroh left a comment •

edited

Loading

andrewkroh Jan 31, 2025 •

edited

Loading

andrewkroh Jan 31, 2025

andrewkroh Jan 31, 2025

andrewkroh Jan 31, 2025

andrewkroh Jan 31, 2025 •

edited

Loading

andrewkroh Jan 31, 2025 •

edited

Loading

jrmolin Jan 31, 2025

jrmolin Jan 31, 2025

Migrate Netflow code to the logp logging library #42270

Are you sure you want to change the base?

Migrate Netflow code to the logp logging library #42270

Conversation

jrmolin commented Jan 8, 2025 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

elasticmachine commented Jan 14, 2025

elasticmachine commented Jan 14, 2025

andrewkroh Jan 15, 2025

Choose a reason for hiding this comment

stefans-elastic commented Jan 29, 2025

jrmolin commented Jan 31, 2025

jrmolin commented Jan 31, 2025

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

andrewkroh Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

andrewkroh Jan 31, 2025

Choose a reason for hiding this comment

andrewkroh Jan 31, 2025

Choose a reason for hiding this comment

andrewkroh Jan 31, 2025

Choose a reason for hiding this comment

andrewkroh Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

andrewkroh Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

jrmolin Jan 31, 2025

Choose a reason for hiding this comment

jrmolin Jan 31, 2025

Choose a reason for hiding this comment

jrmolin commented Jan 8, 2025 •

edited

Loading

andrewkroh left a comment •

edited

Loading

andrewkroh Jan 31, 2025 •

edited

Loading

andrewkroh Jan 31, 2025 •

edited

Loading

andrewkroh Jan 31, 2025 •

edited

Loading