Adjusted logs to be consistent across the code base #237
Conversation
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #237 +/- ##
==========================================
- Coverage 90.19% 89.85% -0.35%
==========================================
Files 61 62 +1
Lines 2753 2710 -43
==========================================
- Hits 2483 2435 -48
- Misses 172 177 +5
Partials 98 98
Continue to review full report at Codecov.
|
pkg/config/sampling/sampling.go
Outdated
| @@ -40,7 +40,10 @@ func (u *Config) Get() *v1.ConfigMap { | |||
| return nil | |||
| } | |||
|
|
|||
| logrus.WithField("instance", u.jaeger.Name).Debug("Assembling the Sampling configmap") | |||
| log.WithFields(log.Fields{ | |||
There was a problem hiding this comment.
I know that you have turned down my opinions but I find the current logging rather ad hoc. I am pretty suer sooner or later somebody will log something without proper fields.
I like static access to logger but in this case I would rather pass it explicitly or use MDC to for consistent logging and avoid repetition every time you access the logger. Another approach is to have a factory method creteLogger(j v1alpha1.Jaeger, tuples...) logger in utils package.
There was a problem hiding this comment.
somebody will log something without proper fields
This happens already, as this PR shows: quite a few places had to be fixed to include the instance and/or namespace.
I'm not against using a base logger for the request-scoped code, but I don't quite like the idea of changing method signatures to include a logger which can be derived from the other parameters (log.WithFields("name": jaeger.Name, "namespace":jaeger.Namespace))
A helper like you proposed would certainly be something to consider, though.
There was a problem hiding this comment.
I added a new commit in the direction of your suggestion. How does it look now?
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
|
Does this resolve #203? My concert in #203 is that the log just says I would expect it log something if there is a difference in desired state. But this PR looks good it's definitelly an improvement. |
|
At the debug level, I think the I see #203 as a general "logging" issue, which is what this PR is solving.
Hopefully done as part of #231 :-) |
Closes #203
Signed-off-by: Juraci Paixão Kröhling [email protected]