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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass MDC tags as Sentry tags #1954

Merged
merged 9 commits into from Mar 28, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -3,6 +3,7 @@
## Unreleased

* Ref: Remove not needed interface abstractions on Android (#1953)
* Feat: Pass MDC tags as Sentry tags (#1954)

## 6.0.0-alpha.3

Expand Down
17 changes: 16 additions & 1 deletion sentry-jul/src/main/java/io/sentry/jul/SentryHandler.java
Expand Up @@ -42,6 +42,7 @@ public class SentryHandler extends Handler {

private @NotNull Level minimumBreadcrumbLevel = Level.INFO;
private @NotNull Level minimumEventLevel = Level.SEVERE;
private @NotNull SentryOptions options;

/** Creates an instance of SentryHandler. */
public SentryHandler() {
Expand All @@ -60,6 +61,7 @@ public SentryHandler(final @NotNull SentryOptions options) {
/** Creates an instance of SentryHandler. */
@TestOnly
SentryHandler(final @NotNull SentryOptions options, final boolean configureFromLogManager) {
this.options = options;
setFilter(new DropSentryFilter());
if (configureFromLogManager) {
retrieveProperties();
Expand Down Expand Up @@ -200,7 +202,20 @@ SentryEvent createEvent(final @NotNull LogRecord record) {
mdcProperties =
CollectionUtils.filterMapEntries(mdcProperties, entry -> entry.getValue() != null);
if (!mdcProperties.isEmpty()) {
event.getContexts().put("MDC", mdcProperties);
if (!options.getMdcTags().isEmpty()) {
for (final String mdcTag : options.getMdcTags()) {
// if mdc tag is listed in SentryOptions, apply as event tag
if (mdcProperties.containsKey(mdcTag)) {
event.setTag(mdcTag, mdcProperties.get(mdcTag));
// remove from all tags applied to logging event
mdcProperties.remove(mdcTag);
Copy link
Member

Choose a reason for hiding this comment

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

@bruno-garcia do we want to add entries only to either tags or context not both?

Copy link
Member

Choose a reason for hiding this comment

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

just in tags is fine, no need to dupe 馃憤

}
}
}
// put the rest of mdc tags in contexts
if (!mdcProperties.isEmpty()) {
event.getContexts().put("MDC", mdcProperties);
}
}
}
event.setExtra(THREAD_ID, record.getThreadID());
Expand Down
19 changes: 18 additions & 1 deletion sentry-jul/src/test/kotlin/io/sentry/jul/SentryHandlerTest.kt
Expand Up @@ -24,14 +24,15 @@ import kotlin.test.assertNull
import kotlin.test.assertTrue

class SentryHandlerTest {
private class Fixture(minimumBreadcrumbLevel: Level? = null, minimumEventLevel: Level? = null, val configureWithLogManager: Boolean = false, val transport: ITransport = mock()) {
private class Fixture(minimumBreadcrumbLevel: Level? = null, minimumEventLevel: Level? = null, val configureWithLogManager: Boolean = false, val transport: ITransport = mock(), mdcTags: List<String>? = null) {
var logger: Logger
var handler: SentryHandler

init {
val options = SentryOptions()
options.dsn = "http://key@localhost/proj"
options.setTransportFactory { _, _ -> transport }
mdcTags?.forEach { options.addMdcTag(it) }
logger = Logger.getLogger("jul.SentryHandlerTest")
handler = SentryHandler(options, configureWithLogManager)
handler.setMinimumBreadcrumbLevel(minimumBreadcrumbLevel)
Expand Down Expand Up @@ -312,6 +313,22 @@ class SentryHandlerTest {
)
}

@Test
fun `sets tags as Sentry tags from MDC`() {
fixture = Fixture(minimumEventLevel = Level.WARNING, mdcTags = listOf("mdcTag1"))
MDC.put("key", "value")
MDC.put("mdcTag1", "mdcTag1Value")
fixture.logger.warning("testing MDC tags")

verify(fixture.transport).send(
checkEvent { event ->
assertEquals(mapOf("key" to "value"), event.contexts["MDC"])
assertEquals(mapOf("mdcTag1" to "mdcTag1Value"), event.tags)
},
anyOrNull()
)
}

@Test
fun `ignore set tags with null values from MDC`() {
fixture = Fixture(minimumEventLevel = Level.WARNING)
Expand Down
4 changes: 2 additions & 2 deletions sentry-log4j2/api/sentry-log4j2.api
Expand Up @@ -4,9 +4,9 @@ public final class io/sentry/log4j2/BuildConfig {
}

public class io/sentry/log4j2/SentryAppender : org/apache/logging/log4j/core/appender/AbstractAppender {
public fun <init> (Ljava/lang/String;Lorg/apache/logging/log4j/core/Filter;Ljava/lang/String;Lorg/apache/logging/log4j/Level;Lorg/apache/logging/log4j/Level;Ljava/lang/Boolean;Lio/sentry/ITransportFactory;Lio/sentry/IHub;)V
public fun <init> (Ljava/lang/String;Lorg/apache/logging/log4j/core/Filter;Ljava/lang/String;Lorg/apache/logging/log4j/Level;Lorg/apache/logging/log4j/Level;Ljava/lang/Boolean;Lio/sentry/ITransportFactory;Lio/sentry/IHub;[Ljava/lang/String;)V
public fun append (Lorg/apache/logging/log4j/core/LogEvent;)V
public static fun createAppender (Ljava/lang/String;Lorg/apache/logging/log4j/Level;Lorg/apache/logging/log4j/Level;Ljava/lang/String;Ljava/lang/Boolean;Lorg/apache/logging/log4j/core/Filter;)Lio/sentry/log4j2/SentryAppender;
public static fun createAppender (Ljava/lang/String;Lorg/apache/logging/log4j/Level;Lorg/apache/logging/log4j/Level;Ljava/lang/String;Ljava/lang/Boolean;Lorg/apache/logging/log4j/core/Filter;Ljava/lang/String;)Lio/sentry/log4j2/SentryAppender;
protected fun createBreadcrumb (Lorg/apache/logging/log4j/core/LogEvent;)Lio/sentry/Breadcrumb;
protected fun createEvent (Lorg/apache/logging/log4j/core/LogEvent;)Lio/sentry/SentryEvent;
public fun start ()V
Expand Down
31 changes: 27 additions & 4 deletions sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java
Expand Up @@ -46,6 +46,7 @@ public class SentryAppender extends AbstractAppender {
private @NotNull Level minimumEventLevel = Level.ERROR;
private final @Nullable Boolean debug;
private final @NotNull IHub hub;
private final @Nullable List<String> mdcTags;

public SentryAppender(
final @NotNull String name,
Expand All @@ -55,7 +56,8 @@ public SentryAppender(
final @Nullable Level minimumEventLevel,
final @Nullable Boolean debug,
final @Nullable ITransportFactory transportFactory,
final @NotNull IHub hub) {
final @NotNull IHub hub,
final @Nullable String[] mdcTags) {
super(name, filter, null, true, null);
this.dsn = dsn;
if (minimumBreadcrumbLevel != null) {
Expand All @@ -67,6 +69,7 @@ public SentryAppender(
this.debug = debug;
this.transportFactory = transportFactory;
this.hub = hub;
this.mdcTags = mdcTags != null ? Arrays.asList(mdcTags) : null;
}

/**
Expand All @@ -87,7 +90,8 @@ public SentryAppender(
@Nullable @PluginAttribute("minimumEventLevel") final Level minimumEventLevel,
@Nullable @PluginAttribute("dsn") final String dsn,
@Nullable @PluginAttribute("debug") final Boolean debug,
@Nullable @PluginElement("filter") final Filter filter) {
@Nullable @PluginElement("filter") final Filter filter,
@Nullable @PluginAttribute("mdcTags") final String mdcTags) {

if (name == null) {
LOGGER.error("No name provided for SentryAppender");
Expand All @@ -101,7 +105,8 @@ public SentryAppender(
minimumEventLevel,
debug,
null,
HubAdapter.getInstance());
HubAdapter.getInstance(),
mdcTags != null ? mdcTags.split(",") : null);
}

@Override
Expand All @@ -117,6 +122,11 @@ public void start() {
}
options.setSentryClientName(BuildConfig.SENTRY_LOG4J2_SDK_NAME);
options.setSdkVersion(createSdkVersion(options));
if (mdcTags != null) {
for (final String mdcTag : mdcTags) {
options.addMdcTag(mdcTag);
}
}
Optional.ofNullable(transportFactory).ifPresent(options::setTransportFactory);
});
} catch (IllegalArgumentException e) {
Expand Down Expand Up @@ -177,7 +187,20 @@ public void append(final @NotNull LogEvent eventObject) {
CollectionUtils.filterMapEntries(
loggingEvent.getContextData().toMap(), entry -> entry.getValue() != null);
if (!contextData.isEmpty()) {
event.getContexts().put("Context Data", contextData);
if (mdcTags != null && !mdcTags.isEmpty()) {
for (final String mdcTag : mdcTags) {
// if mdc tag is listed in SentryOptions, apply as event tag
if (contextData.containsKey(mdcTag)) {
event.setTag(mdcTag, contextData.get(mdcTag));
// remove from all tags applied to logging event
contextData.remove(mdcTag);
}
}
}
// put the rest of mdc tags in contexts
if (!contextData.isEmpty()) {
event.getContexts().put("Context Data", contextData);
}
}

return event;
Expand Down
Expand Up @@ -43,13 +43,13 @@ class SentryAppenderTest {
whenever(transportFactory.create(any(), any())).thenReturn(transport)
}

fun getSut(transportFactory: ITransportFactory? = null, minimumBreadcrumbLevel: Level? = null, minimumEventLevel: Level? = null, debug: Boolean? = null): ExtendedLogger {
fun getSut(transportFactory: ITransportFactory? = null, minimumBreadcrumbLevel: Level? = null, minimumEventLevel: Level? = null, debug: Boolean? = null, mdcTags: List<String>? = null): ExtendedLogger {
if (transportFactory != null) {
this.transportFactory = transportFactory
}
loggerContext.start()
val config: Configuration = loggerContext.configuration
val appender = SentryAppender("sentry", null, "http://key@localhost/proj", minimumBreadcrumbLevel, minimumEventLevel, debug, this.transportFactory, HubAdapter.getInstance())
val appender = SentryAppender("sentry", null, "http://key@localhost/proj", minimumBreadcrumbLevel, minimumEventLevel, debug, this.transportFactory, HubAdapter.getInstance(), mdcTags?.toTypedArray())
config.addAppender(appender)

val ref = AppenderRef.createAppenderRef("sentry", null, null)
Expand Down Expand Up @@ -241,6 +241,22 @@ class SentryAppenderTest {
)
}

@Test
fun `sets tags from ThreadContext as Sentry tags`() {
val logger = fixture.getSut(minimumEventLevel = Level.WARN, mdcTags = listOf("mdcTag1"))
ThreadContext.put("key", "value")
ThreadContext.put("mdcTag1", "mdcTag1Value")
logger.warn("testing MDC tags")

verify(fixture.transport).send(
checkEvent { event ->
assertEquals(mapOf("key" to "value"), event.contexts["Context Data"])
assertEquals(mapOf("mdcTag1" to "mdcTag1Value"), event.tags)
},
anyOrNull()
)
}

@Test
fun `ignore set tags with null values from ThreadContext`() {
val logger = fixture.getSut(minimumEventLevel = Level.WARN)
Expand Down
Expand Up @@ -112,7 +112,20 @@ protected void append(@NotNull ILoggingEvent eventObject) {
CollectionUtils.filterMapEntries(
loggingEvent.getMDCPropertyMap(), entry -> entry.getValue() != null);
if (!mdcProperties.isEmpty()) {
event.getContexts().put("MDC", mdcProperties);
if (!options.getMdcTags().isEmpty()) {
for (final String mdcTag : options.getMdcTags()) {
// if mdc tag is listed in SentryOptions, apply as event tag
if (mdcProperties.containsKey(mdcTag)) {
event.setTag(mdcTag, mdcProperties.get(mdcTag));
// remove from all tags applied to logging event
mdcProperties.remove(mdcTag);
}
}
}
// put the rest of mdc tags in contexts
if (!mdcProperties.isEmpty()) {
event.getContexts().put("MDC", mdcProperties);
}
}

return event;
Expand Down
Expand Up @@ -31,7 +31,7 @@ import kotlin.test.assertNull
import kotlin.test.assertTrue

class SentryAppenderTest {
private class Fixture(dsn: String? = "http://key@localhost/proj", minimumBreadcrumbLevel: Level? = null, minimumEventLevel: Level? = null) {
private class Fixture(dsn: String? = "http://key@localhost/proj", minimumBreadcrumbLevel: Level? = null, minimumEventLevel: Level? = null, mdcTags: List<String>? = null) {
val logger: Logger = LoggerFactory.getLogger(SentryAppenderTest::class.java)
val loggerContext = LoggerFactory.getILoggerFactory() as LoggerContext
val transportFactory = mock<ITransportFactory>()
Expand All @@ -43,6 +43,7 @@ class SentryAppenderTest {
val appender = SentryAppender()
val options = SentryOptions()
options.dsn = dsn
mdcTags?.forEach { options.addMdcTag(it) }
appender.setOptions(options)
appender.setMinimumBreadcrumbLevel(minimumBreadcrumbLevel)
appender.setMinimumEventLevel(minimumEventLevel)
Expand Down Expand Up @@ -217,6 +218,22 @@ class SentryAppenderTest {
)
}

@Test
fun `sets tags as sentry tags from MDC`() {
fixture = Fixture(minimumEventLevel = Level.WARN, mdcTags = listOf("mdcTag1"))
MDC.put("key", "value")
MDC.put("mdcTag1", "mdcTag1Value")
fixture.logger.warn("testing MDC tags")

verify(fixture.transport).send(
checkEvent { event ->
assertEquals(mapOf("key" to "value"), event.contexts["MDC"])
assertEquals(mapOf("mdcTag1" to "mdcTag1Value"), event.tags)
},
anyOrNull()
)
}

@Test
fun `ignore set tags with null values from MDC`() {
fixture = Fixture(minimumEventLevel = Level.WARN)
Expand Down
Expand Up @@ -3,3 +3,4 @@ dsn=https://502f25099c204a2fbf4cb16edc5975d1@o447951.ingest.sentry.io/5428563
debug=true
environment=staging
in-app-includes=io.sentry.samples
mdc-tags=userId,requestId
Expand Up @@ -11,6 +11,8 @@
dsn="https://502f25099c204a2fbf4cb16edc5975d1@o447951.ingest.sentry.io/5428563"
minimumBreadcrumbLevel="DEBUG"
minimumEventLevel="WARN"
debug="true"
mdcTags="userId"
Copy link
Member

Choose a reason for hiding this comment

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

should this sample also show how to set multiple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sample updated.

/>
</Appenders>
<Loggers>
Expand Down
Expand Up @@ -11,6 +11,7 @@
<debug>true</debug>
<!-- NOTE: Replace the test DSN below with YOUR OWN DSN to see the events from this app in your Sentry project/dashboard -->
<dsn>https://502f25099c204a2fbf4cb16edc5975d1@o447951.ingest.sentry.io/5428563</dsn>
<mdcTag>userId</mdcTag>
Copy link
Member

Choose a reason for hiding this comment

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

should this sample also show how to set multiple?

Copy link
Member

Choose a reason for hiding this comment

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

is <mdcTag> correct here? or should it be <mdcTags>?

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 is correct. under the hood it calls addMdcTag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sample updated.

</options>
<!-- Demonstrates how to modify the minimum values -->
<!-- Default for Events is ERROR -->
Expand Down
4 changes: 4 additions & 0 deletions sentry/api/sentry.api
Expand Up @@ -127,6 +127,7 @@ public final class io/sentry/ExternalOptions {
public fun addIgnoredExceptionForType (Ljava/lang/Class;)V
public fun addInAppExclude (Ljava/lang/String;)V
public fun addInAppInclude (Ljava/lang/String;)V
public fun addMdcTag (Ljava/lang/String;)V
public fun addTracingOrigin (Ljava/lang/String;)V
public static fun from (Lio/sentry/config/PropertiesProvider;Lio/sentry/ILogger;)Lio/sentry/ExternalOptions;
public fun getDebug ()Ljava/lang/Boolean;
Expand All @@ -139,6 +140,7 @@ public final class io/sentry/ExternalOptions {
public fun getInAppExcludes ()Ljava/util/List;
public fun getInAppIncludes ()Ljava/util/List;
public fun getMaxRequestBodySize ()Lio/sentry/SentryOptions$RequestSize;
public fun getMdcTags ()Ljava/util/List;
public fun getPrintUncaughtStackTrace ()Ljava/lang/Boolean;
public fun getProguardUuid ()Ljava/lang/String;
public fun getProxy ()Lio/sentry/SentryOptions$Proxy;
Expand Down Expand Up @@ -998,6 +1000,7 @@ public class io/sentry/SentryOptions {
public fun addInAppExclude (Ljava/lang/String;)V
public fun addInAppInclude (Ljava/lang/String;)V
public fun addIntegration (Lio/sentry/Integration;)V
public fun addMdcTag (Ljava/lang/String;)V
public fun addScopeObserver (Lio/sentry/IScopeObserver;)V
public fun addTracingOrigin (Ljava/lang/String;)V
public fun getBeforeBreadcrumb ()Lio/sentry/SentryOptions$BeforeBreadcrumbCallback;
Expand Down Expand Up @@ -1026,6 +1029,7 @@ public class io/sentry/SentryOptions {
public fun getMaxQueueSize ()I
public fun getMaxRequestBodySize ()Lio/sentry/SentryOptions$RequestSize;
public fun getMaxSpans ()I
public fun getMdcTags ()Ljava/util/List;
public fun getOutboxPath ()Ljava/lang/String;
public fun getPrintUncaughtStackTrace ()Ljava/lang/Boolean;
public fun getProguardUuid ()Ljava/lang/String;
Expand Down
12 changes: 12 additions & 0 deletions sentry/src/main/java/io/sentry/ExternalOptions.java
Expand Up @@ -32,6 +32,7 @@ public final class ExternalOptions {
private final @NotNull List<String> inAppExcludes = new CopyOnWriteArrayList<>();
private final @NotNull List<String> inAppIncludes = new CopyOnWriteArrayList<>();
private final @NotNull List<String> tracingOrigins = new CopyOnWriteArrayList<>();
private final @NotNull List<String> mdcTags = new CopyOnWriteArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Can anyone think of a name that's a bit more generic than MDC Tags?
In case any other SDK wants to add support for a similar feature it might be nice if they can use the same name.
Something like logContextTags or promoteLogContextEntriesToTags?

We should then probably mention somewhere that this takes MDC entries and puts them into sentry tags so people can actually find this feature when searching for MDC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps contextTags? I am open to change it to anything more suitable.

Copy link
Member

Choose a reason for hiding this comment

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

contextTags sounds good to me. Can you please rename it? Sorry for the inconvenience but I'd rather change it now than deprecate and rename later as we've already been asked to pick a name that can be used for more than just the Java SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

private @Nullable String proguardUuid;
private final @NotNull Set<Class<? extends Throwable>> ignoredExceptionsForType =
new CopyOnWriteArraySet<>();
Expand Down Expand Up @@ -81,6 +82,9 @@ public final class ExternalOptions {
for (final String tracingOrigin : propertiesProvider.getList("tracing-origins")) {
options.addTracingOrigin(tracingOrigin);
}
for (final String mdcTag : propertiesProvider.getList("mdc-tags")) {
options.addMdcTag(mdcTag);
}
options.setProguardUuid(propertiesProvider.getProperty("proguard-uuid"));

for (final String ignoredExceptionType :
Expand Down Expand Up @@ -212,6 +216,10 @@ public void setProxy(final @Nullable SentryOptions.Proxy proxy) {
return inAppIncludes;
}

public @NotNull List<String> getMdcTags() {
return mdcTags;
}

public @Nullable String getProguardUuid() {
return proguardUuid;
}
Expand All @@ -236,6 +244,10 @@ public void addTracingOrigin(final @NotNull String tracingOrigin) {
this.tracingOrigins.add(tracingOrigin);
}

public void addMdcTag(final @NotNull String mdcTag) {
this.mdcTags.add(mdcTag);
}

public void addIgnoredExceptionForType(final @NotNull Class<? extends Throwable> exceptionType) {
this.ignoredExceptionsForType.add(exceptionType);
}
Expand Down