Standardise log levels#1150
Conversation
0686afd to
4dbb31d
Compare
| if (count >= _maxTracesPerInterval) | ||
| { | ||
| Log.Debug("Dropping trace id {0} with count of {1} for last {2}ms.", span.TraceId, count, _intervalMilliseconds); | ||
| Log.Warning("Dropping trace id {TraceId} with count of {Count} for last {Interval}ms.", span.TraceId, count, _intervalMilliseconds); |
There was a problem hiding this comment.
This may be very noisy if it's a warning level. Many applications exceed the maximum.
There was a problem hiding this comment.
With the rate limiting API, (in a separate PR atm) this shouldn't be a problem
lucaspimentel
left a comment
lucaspimentel
left a comment
There was a problem hiding this comment.
Replacing {0},{1} with {name1},{name2} is great! This always bothered me and I don't remember why we didn't do this before.
If you split that change into a separate PR, I would approve it on the spot. However, some of the other changes in this PR need further discussion. I think we need to keep the Information level around for log messages we want to emit by default but are not Warning or Error. It may be time to update that RFC 😅
| } | ||
|
|
||
| Log.Debug("ISpanBuilder.AddReference is not implemented for other references than ChildOf by Datadog.Trace"); | ||
| Log.Warning("ISpanBuilder.AddReference is not implemented for other references than ChildOf by Datadog.Trace"); |
There was a problem hiding this comment.
It may sound weird, but in the OpenTracing library we intentionally don't want to log warnings when unimplemented features are used (other examples). We originally logged them, but we received user complaints about spamming their logs. Many times users combine this library with another 3rd party library they may use features like ISpan.Log() which the user cannot disable.
There was a problem hiding this comment.
Then again, if we're adding rate limiting, spamming the logs should not be an issue anymore.
There was a problem hiding this comment.
Yeah, I agree, it was just that it was explicitly called out in the RFC (as with many of the other issues here) 🙂
There was a problem hiding this comment.
Left it as debug for now - doesn't seem worth updating to warning if people have actively complained about it!
| if (!success) | ||
| { | ||
| Log.Debug("Trace buffer is full. Dropping a trace from the buffer."); | ||
| Log.Warning("Trace buffer is full. Dropping a trace from the buffer."); |
There was a problem hiding this comment.
We should probably check with the other tracer teams whether we should log this message as debug or warning. It's expected that we never send more than 1,000 tracer per second to the Agent. (There's an undocumented setting to change this number.)
| if (count >= _maxTracesPerInterval) | ||
| { | ||
| Log.Debug("Dropping trace id {0} with count of {1} for last {2}ms.", span.TraceId, count, _intervalMilliseconds); | ||
| Log.Warning("Dropping trace id {TraceId} with count of {Count} for last {Interval}ms.", span.TraceId, count, _intervalMilliseconds); |
There was a problem hiding this comment.
Similar to other comments I left, we should check with the larger APM Integrations team about the severity of this message.
@lucaspimentel I created a separate PR here: #1163 |
The RFC only uses Debug, Warn, and Error. We add Information, for emitting non-error logs (such as configuration status logs)
4dbb31d to
55dc8b1
Compare
4 participants
Initial cleanup of log levels before implementing remainder of the logging RFC
Debug,Warning,Errorstring.Format()style log templates ("{0},{1}") with named arguments ("{Name},{Value}"). Doesn't make much difference atm, as we're only writing to file, but is better practice, and will be better if we do structured logging down the line instead.In a follow up PR I'll encapsulate the
ILoggerto restrict to using only the allowed levels, and add rate limiting, as defined in the RFC@DataDog/apm-dotnet