-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: Added logging #74
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thulasirajkomminar thanks for your PR 👍
We want to keep the client source base as simple as possible, so the logging infrastructure should be also simple.
What do you think about add simple logger implementation like following code?
client.go
:
var logger *log.Logger
var logLevels = map[string]bool{
"debug": true,
"info": true,
"warn": true,
"error": true,
}
// ConfigureLogger initializes logger with specified log levels.
// Supported levels: debug, info, warn, error.
func ConfigureLogger(levels map[string]bool) {
for level, enabled := range levels {
if _, exists := logLevels[level]; exists {
logLevels[level] = enabled
}
}
}
initialialize this logger in New
function:
logger = log.New(os.Stdout, "influxdb3-go: ", log.LstdFlags)
ConfigureLogger(map[string]bool{"debug": true, "info": true, "warn": false, "error": true})
and use it in point_values.go
:
func (pv *PointValues) SetTag(name string, value string) *PointValues {
if value == "" {
if enabled, ok := logLevels["debug"]; ok && enabled {
logger.Printf("Empty tags has no effect, tag [%s], measurement [%s]\n", name, pv.MeasurementName)
}
} else {
pv.Tags[name] = value
}
return pv
}
also you can enhance the above implementation with enum-like-constants:
type LogLevel int
const (
Debug LogLevel = iota
Info
Warn
Error
)
@bednar Thought about just adding a simple logger but then I wanted to follow the V2 standards :) . So I like this simple implementation so modified my commit. |
0220155
to
5939197
Compare
Good catch! 👍 You are right it should be a part of ClientConfig. |
13303c6
to
e56e63e
Compare
@bednar could you verify? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simplify our codebase we can use default logger:
@bednar Could you have a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thulasirajkomminar thanks again for your PR 👍
There is only little change in CHANGELOG.md. After that we will be able to merge PR:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
We will merge the PR after we fix our CI to support PRs from forked repository.
I think the PR needs to be rebased or main merged into it to use fixed CI and have Code coverage check will probably fail because code coverage will decrease as there is quite a few lines not covered by unit tests (#80), but that could be ignored for now... |
Co-authored-by: Jakub Bednář <jakub.bednar@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
- Coverage 83.09% 74.54% -8.56%
==========================================
Files 12 12
Lines 994 994
==========================================
- Hits 826 741 -85
- Misses 138 228 +90
+ Partials 30 25 -5 ☔ View full report in Codecov by Sentry. |
Closes #73
Proposed Changes
Briefly describe your proposed changes:
Added log package to solve issue #73
Checklist