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

Add Core function to logp.Logger to allow access to backing zapcore.Core. #88

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

blakerouse
Copy link
Contributor

@blakerouse blakerouse commented Nov 5, 2022

What does this PR do?

Adds the ability to retrieve the zapcore.Core from the logp.Logger. This allows the ability to write entries directly into the logger bypassing any log levels settings.

Why is it important?

This is needed for the work on elastic/elastic-agent#221. The io.Writer connected to stdout and stderr from the spawned subprocesses will read the output from the process and output it directly to the zapcore.Core. That is required because each component and unit can have its own log level different from the Elastic Agent log level. Because of that design it requires that log messages bypass the log level settings for the logger used by Elastic Agent.

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
  • I have added an entry in CHANGELOG.md

Related issues

@blakerouse blakerouse added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Nov 5, 2022
@blakerouse blakerouse requested a review from a team November 5, 2022 17:20
@blakerouse blakerouse requested a review from a team as a code owner November 5, 2022 17:20
@blakerouse blakerouse self-assigned this Nov 5, 2022
@blakerouse blakerouse requested review from belimawr and rdner and removed request for a team November 5, 2022 17:20
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-05T17:20:59.849+0000

  • Duration: 9 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 723
Skipped 5
Total 728

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@@ -224,6 +224,11 @@ func (l *Logger) Sync() error {
return l.logger.Sync()
}

// Core returns the backend zapcore.Core for the logger.
func (l *Logger) Core() zapcore.Core {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be better to define a type alias like it's done with the zap.Option:

// LogOption configures a Logger.
type LogOption = zap.Option

I don't know how far we want to go exposing the zap types in our library. I see that the Logger struct just has zap types directly:

// Logger logs messages to the configured output.
type Logger struct {
logger *zap.Logger
sugar *zap.SugaredLogger
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't see a need to hide it. It is zap and zapcore under the hood and the go.mod for any project that imports elastic-agent-libs will show zap and zapcore.

Copy link
Member

Choose a reason for hiding this comment

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

It would rather an abstraction should we decide to switch from zap to something else. I guess that's the point. But I'm okay with this.

@blakerouse blakerouse merged commit a3029bb into elastic:main Nov 7, 2022
@blakerouse blakerouse deleted the logp-expose-zapcore branch November 7, 2022 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elastic Agent should aggregate the log from his subprocesses
3 participants