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

[Chore] Add functions required for single logger instance #297

Merged
merged 13 commits into from
Mar 25, 2025

Conversation

khushijain21
Copy link
Contributor

@khushijain21 khushijain21 commented Mar 19, 2025

What does this PR do?

Required for https://github.com/elastic/ingest-dev/issues/5251

This PR adds some existing methods (on logp package) but with Local appended to the method name. These are required by libbeat to use single logger instance. It does not remove any existing methods not to break any code dependent on the logp library

Why is it important?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

Related issues

Sorry, something went wrong.

@khushijain21 khushijain21 requested a review from a team as a code owner March 19, 2025 08:36
@khushijain21 khushijain21 requested review from belimawr and rdner and removed request for a team March 19, 2025 08:36
Copy link
Member

@AndersonQ AndersonQ left a comment

Choose a reason for hiding this comment

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

@khushijain21 for the function which need docs, if the doc would be the same as for the non-local version you can refer to the non-local and point out the difference. Like that:

// SomeFunction performs a specific operation and logs relevant information using the global logger.
//
// It takes an input string and processes it, logging the input and the result of the processing.
func SomeFunction(input string) string {


// SomeFunctionLocal is the same as SomeFunction but returns a logger instance instead.
func SomeFunctionLocal(input string, logger *log.Logger) (string, *log.Logger) {

@khushijain21
Copy link
Contributor Author

khushijain21 commented Mar 19, 2025

With new local methods, we will lose the observerLogs parameter available on global config.

Unless we change Logger struct to include

type Logger struct {
	logger       *zap.Logger
	sugar        *zap.SugaredLogger
	observerLogs *observer.ObservedLogs. // additional
}

This would change many API's that depend on Logger.

Instead, users can get similar functionality with this

	observed, zapLogs := observer.New(zapcore.DebugLevel)
	observer, err := logp.ConfigureWithCoreLocal(defaultConfig, observed)

Any thoughts? I prefer going with the second option.

@mauri870
Copy link
Member

Any thoughts? I prefer going with the second option.

I like the second option too. First option would require a testing-only struct field wich does not make much sense for production code. Second option is more pluggable as well.

@rdner
Copy link
Member

rdner commented Mar 19, 2025

@khushijain21 I thought [observerLogs](https://github.com/elastic/elastic-agent-libs/blob/main/logp/core.go#L63) was just for testing purposes and validating logs after a test? Is it used elsewhere?

If we change all our interfaces to accept a local logger, testing should also do so and everything should be recorded in the local logger instead.

@khushijain21
Copy link
Contributor Author

khushijain21 commented Mar 19, 2025

observerLogs was just for testing purposes and validating logs after a test? Is it used elsewhere?

Yes it is used only for testing. It is used in 9 files in beats https://github.com/search?q=repo%3Aelastic%2Fbeats%20ObserverLogs&type=code

If we change all our interfaces to accept a local logger, testing should also do so and everything should be recorded in the local logger instead.

Yes, so users can create an observer core and access "testing logs" on the local logger. As described on the second part here #297 (comment)

rdner
rdner previously approved these changes Mar 19, 2025
@leehinman
Copy link
Contributor

I'd really like @belimawr to take a look before we merge. I think it is safe since we are only adding functions, but logp has a history of unexpected behavior.

Co-authored-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
khushijain21 and others added 4 commits March 21, 2025 12:20
Co-authored-by: Tiago Queiroz <me@tiago.life>
Co-authored-by: Tiago Queiroz <me@tiago.life>
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@khushijain21 khushijain21 self-assigned this Mar 25, 2025
@khushijain21 khushijain21 merged commit 9f3278b into elastic:main Mar 25, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants