Skip to content

Always set log level on telemetry logs #8916

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Always set log level on telemetry logs #8916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions internal-api/src/main/java/datadog/trace/api/telemetry/LogCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
import org.slf4j.Marker;
import org.slf4j.MarkerFactory;

/**
* A collector for telemetry log messages. Most consumers should not use this class directly, but rather
* use SLF4J logging, which will call this collector automatically.
*/
public class LogCollector {
public static final Marker SEND_TELEMETRY = MarkerFactory.getMarker("SEND_TELEMETRY");
public static final Marker EXCLUDE_TELEMETRY = MarkerFactory.getMarker("EXCLUDE_TELEMETRY");
Expand All @@ -34,6 +38,12 @@ private LogCollector() {
this.rawLogMessages = new ConcurrentHashMap<>(maxCapacity);
}

/**
* Schedule a log message for processing by the telemetry subsystem.
* @param logLevel ERROR, WARN or DEBUG. Other values are accepted, but will be normalized to DEBUG. Cannot be null.
* @param message The log message, needs to be free from sensitive data. Cannot be null.
* @param throwable An optional throwable to associate a stacktrace with the log message, can be null.
*/
public void addLogMessage(String logLevel, String message, Throwable throwable) {
if (rawLogMessages.size() >= maxCapacity) {
// TODO: We could emit a metric for dropped logs.
Expand Down Expand Up @@ -69,6 +79,10 @@ public Collection<RawLogMessage> drain() {
return list;
}

/**
* A log message scheduled for processing by the telemetry subsystem.
* Consumers should not use this class directly.
*/
public static class RawLogMessage {
public final String message;
public final String logLevel;
Expand Down
10 changes: 4 additions & 6 deletions telemetry/src/main/java/datadog/telemetry/api/LogMessageLevel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,21 @@
package datadog.telemetry.api;

import javax.annotation.Nullable;

public enum LogMessageLevel {
ERROR,
WARN,
DEBUG;

@Nullable
public static LogMessageLevel fromString(String value) {
if (value == null) {
return DEBUG;
}
switch (value) {
case "ERROR":
return ERROR;
case "WARN":
return WARN;
case "DEBUG":
return DEBUG;
default:
return null;
return DEBUG;
}
}
}
5 changes: 1 addition & 4 deletions telemetry/src/main/java/datadog/telemetry/log/LogPeriodicAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,9 @@ public void doIteration(TelemetryService service) {
new LogMessage()
.message(rawLogMsg.message)
.tracerTime(rawLogMsg.timestamp)
.level(LogMessageLevel.fromString(rawLogMsg.logLevel))
.count(rawLogMsg.count);

if (rawLogMsg.logLevel != null) {
logMessage.level(LogMessageLevel.fromString(rawLogMsg.logLevel));
}

if (rawLogMsg.throwable != null) {
logMessage.stackTrace(renderStackTrace(rawLogMsg.throwable));
}
Expand Down
37 changes: 37 additions & 0 deletions telemetry/src/test/groovy/datadog/telemetry/log/LogPeriodicActionTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,34 @@ class LogPeriodicActionTest extends DDSpecification {
LogCollector.get().drain()
}

void 'log with unknown log level is normalized to DEBUG'() {
LogMessage logMessage

when:
LogCollector.get().addLogMessage("INFO", "test", null)
periodicAction.doIteration(telemetryService)

then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.DEBUG
logMessage.getMessage() == 'test'
}

void 'log with null log level is normalized to DEBUG'() {
LogMessage logMessage

when:
LogCollector.get().addLogMessage(null, "test", null)
periodicAction.doIteration(telemetryService)

then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.DEBUG
logMessage.getMessage() == 'test'
}

Comment on lines +24 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: how about to combine into one test using where: part?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense.

Anyway, I'm holding off, as an alternative might be accepting INFO in the backend. The log level exists there, but it's currently rejected at intake.

void 'log with datadog throwable'() {
LogMessage logMessage

Expand All @@ -34,6 +62,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() == "${MutableException.canonicalName}: exception\n" +
" at datadog.MyClass.method(file:42)\n"
Expand All @@ -52,6 +81,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() == "${MutableException.canonicalName}\n" +
" at java.MyClass.method(file:42)\n"
Expand All @@ -70,6 +100,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() == "${MutableException.canonicalName}\n"
}
Expand All @@ -85,6 +116,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getCount() == 2
}
Expand All @@ -104,6 +136,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() != null
logMessage.getCount() == 2
Expand All @@ -129,6 +162,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() == "${MutableException.canonicalName}\n" +
" at (redacted)\n" +
Expand Down Expand Up @@ -162,6 +196,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() == "${MutableException.canonicalName}\n" +
" at (redacted)\n" +
Expand Down Expand Up @@ -204,6 +239,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() == "${MutableException.canonicalName}\n" +
" at java.MyClass.method(file:42)\n" +
Expand Down Expand Up @@ -240,6 +276,7 @@ class LogPeriodicActionTest extends DDSpecification {
then:
1 * telemetryService.addLogMessage(_) >> { args -> logMessage = args[0] }
0 * _
logMessage.getLevel() == LogMessageLevel.ERROR
logMessage.getMessage() == 'test'
logMessage.getStackTrace() == "${MutableException.canonicalName}\n" +
" at java.MyClass.method(file:42)\n" +
Expand Down
Loading