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

configure logging from a zap.Core #218

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Conversation

leehinman
Copy link
Contributor

What does this PR do?

Adds new ConfigureWithCore to the logp package. This allows you to use an existing zapcore.Core instead of creating a new one. This is useful if you need to use logp within a project that has already setup a zapcore.Core on it's own.

Why is it important?

Allows projects that use logp for logging to use an existing zapcore.Core

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

Author's Checklist

  • [ ]

Related issues

@leehinman leehinman added the enhancement New feature or request label Jul 8, 2024
@leehinman leehinman requested a review from a team as a code owner July 8, 2024 15:06
@leehinman leehinman requested review from AndersonQ, rdner and belimawr and removed request for a team July 8, 2024 15:06
@leehinman leehinman added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jul 8, 2024
logp/core.go Outdated
@@ -151,6 +151,56 @@ func ConfigureWithOutputs(defaultLoggerCfg Config, outputs ...zapcore.Core) erro
return nil
}

// ConfigureWithCore configures the global logger to use an output created
// from `defaultLoggerCfg` and all the output passed by `output`.
func ConfigureWithCore(defaultLoggerCfg Config, core zapcore.Core) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

@leehinman, I'm confused here, how is this function conceptually different from ConfigureWithOutputs?

Even the GoDoc for it is pretty much the same :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

observedLogs and the default logger output. But yes, these are very similar functions. I'm fine with saying we should refactor. The reason for opening the PR was to get you to look at it :-)

@cmacknz
Copy link
Member

cmacknz commented Jul 9, 2024

Is there a unit test that could be written for this function?

@leehinman leehinman marked this pull request as draft July 11, 2024 14:04
@leehinman
Copy link
Contributor Author

Converted to draft PR due to PTO and I think Tiago and I need to talk through if we can just use ConfigureWithOutputs.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@leehinman leehinman marked this pull request as ready for review September 17, 2024 19:51
@@ -151,6 +151,48 @@ func ConfigureWithOutputs(defaultLoggerCfg Config, outputs ...zapcore.Core) erro
return nil
}

// ConfigureWithCore configures the global logger to use the passed in
// core. It is assumed that an output has already been defined with
Copy link
Member

Choose a reason for hiding this comment

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

[NIT]
Is there any reason to use double spaces after the period?

Copy link
Member

Choose a reason for hiding this comment

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

I recall seeing this in some other PRs from Lee, maybe it is an editor configuration on his side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just for context, since you both seem interested. I'm not getting into a flame war about white space formatting in comments.

It has to do with being old enough that I learned to type on an actual typewriter. 2 spaces between sentences were required. To see why they were required take a look at a full sheet of text in Pica 10pt (Courier is pretty close), and make sure justification is disabled. Without the 2 spaces it is very hard to scan text to see where one sentence ends and one begins. And since you are using your eyes to "search", this is a very desirable feature.

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.

LGTM, but you may want to see if Tiago still has any comments

@leehinman leehinman merged commit 4babd25 into elastic:main Sep 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 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.

6 participants