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.

Sign up for GitHub

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

Jump to bottom

Instrument error logs #522

Merged
merged 5 commits into from Nov 16, 2023
Merged

Instrument error logs #522

merged 5 commits into from Nov 16, 2023

Conversation

mmatczuk
Copy link
Contributor

No description provided.

@what-the-diff What The Diff
Copy link

what-the-diff bot commented Nov 14, 2023

PR Summary

  • Introduction of Metrics Function
    The team added a function 'registerErrorsMetric()' which will track errors in a more organized manner.

  • New File opts.go Addition
    The addition of a new file opts.go is aimed at improving options for modifying the log message and handling errors.

  • Enhancement of New() Function
    The team has improved the 'New()' function to accept additional options for configuring instances, providing more flexibility.

  • Modification of the Logger struct fields
    The Logger structure was modified to now include fields for error, info, and debug prefixes, enhancing the capacity for messages and dealing with errors.

  • Modified Named() function in Logger
    They adjusted an existing functionality 'Named()' that updates the prefixes based on provided names, optimizing error, info, and debug prefixes management.

  • Adjusted Functions in Logger
    The functions Errorf(), Infof(), and Debugf() within Logger have been adjusted to use the updated prefixes and handle log message decoration.

  • New Unwrap() function in Logger
    A new function 'Unwrap()' has been introduced to return the underlying 'log.Logger' pointer, providing easier access to it.

Choraden
Choraden reviewed Nov 14, 2023
View reviewed changes
cmd/forwarder/run/run.go Outdated
return nil, err
}

return func(name, format string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: format is not used -> _

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have a linter for that? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for the future, probably not worth it.

Choraden
Choraden approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@Choraden Choraden left a comment

Choose a reason for hiding this comment

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

LGTM

nit: I would squash log/stdlog: add OnError() with log/stdlog: add name and format to onError, I don't see a value of keeping them split.

@mmatczuk
Copy link
Contributor Author

Yeah I should rewrite it.

@mmatczuk
log/stdlog: refactor exported fields to functional options
62d0778 
This reduces the scope of the fields and avoids data races.
@mmatczuk
log/stdlog: precompute format prefixes
ef92031 
This allows to keep the logger name and avoid mutlielement concatenation on every log.
@mmatczuk
log/stdlog: use pointer receivers
d7c9fcb 
After adding more fields the Logger has become quite heavy for a copy on every call.
@mmatczuk
log/stdlog: add onError() listener
d1be995 
There are multiple options to gather information about error being logged.
Adding a listener has a minimal impact on the API and runtime.
It does not affect performance of Debug logs in any way.
@mmatczuk
cmd/forwarder/run: add errors metric
ece1cab 
# HELP forwarder_errors_total Number of errors
# TYPE forwarder_errors_total counter
forwarder_errors_total{name="A"} 1
forwarder_errors_total{name="B"} 1

Fixes #487
@mmatczuk
Copy link
Contributor Author

Sent patchset v2

@mmatczuk mmatczuk merged commit eb21c2c into main Nov 16, 2023
5 checks passed
@mmatczuk mmatczuk deleted the mmt/instrument_error_logs branch November 16, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Choraden Choraden Choraden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mmatczuk @Choraden