Feature/add tags to forwarded logs #1177

christopher-gilbert · 2023-03-10T08:46:10Z

Full details of the changes are at #1104 - important points about the change and approach are repeated here:

I use the Java agent, and would like additional static information (git branch/commit hash) that I have available in environment variables to be made available as a dimension in Log events. The labels configuration allows me to easily inject that information into events other than forwarded log events, where they are visible with tags. prefix, I would like those tags to be included in logs that are forwarded by the agent.

I note that @StanlieK contributed an enhancement to include MDC in log events (#866) - this is one option but MDC isn't really the right tool here - the information I have is the same for all threads, and using MDC would mean ensuring that it gets set in every thread (http request handling, background jobs that fire etc) - that is hard to do reliably and doesn't feel right given that the value is fixed at runtime.

The rationale for the changes I made is as follows:

I note that agent-bridge can only access config via the flattened properties rather than, say, accessing LabelsConfig and so the approach I took is to extend the existing flattening approach, which currently will parse Strings into Boolean or Integer. My changes preserve the existing behaviour, and extend it to cover List<String> and Map<String, String> using the existing parsing logic used within specific config types.

I have extracted map parsing from LabelsConfigImpl into BaseConfig so that List and Map parsing are now in the same place, and to avoid applying labels specific validation to any other arbitrary 'String as Map' data - the parsing approach could be extended to pass validation rules in, or define a static map of keys -> validators but I decided to keep things simple.

In terms of preserving existing behaviour, although the flattening doesn't apply any validation, the config is still also parsed into LabelsConfigImpl and so labels are still validated (and all existing unit tests pass). The only behaviour change I have made is to trim the keys and values BEFORE the length is validated - previously trimming whitespace happened afterwards - I think it is more correct to validate the value that is actually sent by the agent - at the moment we might see a warning that labels have been truncated to 255 characters but the value could be shorter if the 255 included leading/trailing whitespace. I can amend this though if needed to avoid any change.

Another point to note is that only the tags that are defined in labels config are added to forwarded Log metadata, none of the tags that are added by the agent (eg instrumentation.name, instrumentation.provider etc) are included so it means forwarded logs aren't quite consistent with other entities, but looking at the code I couldn't see those tags being applied within the agent so I'm wondering if they are added at the server end? The config tags are fine for our use case but if there is a need to garnish forwarded logs with other tags, I'd need a steer on where that information is added to other entities.

are flattened during loading so that they can be retrieved as useful types without additional parsing during retrieval. This change effectively preserves existing behaviour for handling Strings, Booleans and Integers in flattened configuration while extending the behaviour to Lists and Maps. Semantics for interpreting Strings are taken from AgentConfigImpl in the case of currently supported types, and structured configurations (AttributesConfig and LabelsConfig) for new types.

general parsing of Strings to Maps does not have labels specific validation applied. Applying tag feature to Logback. General tidy up.

Update LabelsConfigImplTest with potential parse failure scenario. Break up large method in Logback and Log4J2 instrumentation, as was done in the Log4J1 PR that added MDC. Renaming of a couple of constants to reflect what they represent rather than their value, to avoid longer term confusion if, for example, we choose to allow multiple delimiters.

…d-logs Fix conflicts by adding the instrumentation attribute to the event map of agents that had been updated on feature branch

Add tags, and apply tidy up that was introduced in a previous PR and has now been applied to all logging agents for consistency.

CLAassistant · 2023-03-10T08:46:18Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

christopher-gilbert · 2023-03-11T12:16:10Z

agent-bridge/src/main/java/com/newrelic/agent/bridge/logging/LogAttributeType.java

@@ -14,7 +14,8 @@ public String applyPrefix(String key) {
            return key;
        }
    },
-    CONTEXT(AppLoggingUtils.CONTEXT_DATA_ATTRIBUTE_PREFIX);
+    CONTEXT(AppLoggingUtils.CONTEXT_DATA_ATTRIBUTE_PREFIX),


extending the changes introduce by @meiao who introduced MDC support

christopher-gilbert · 2023-03-11T12:16:29Z

agent-bridge/src/test/java/com/newrelic/agent/bridge/logging/AppLoggingUtilsTest.java

 import org.junit.Assert;
 import org.junit.Test;

-import static org.mockito.ArgumentMatchers.any;


Just tidying up some unused imports

christopher-gilbert · 2023-03-11T12:17:37Z

...tation/apache-log4j-1/src/main/java/com/nr/agent/instrumentation/log4j1/LoggingEventMap.java

        addLoggerInfo(event, logEventMap);
-        addMessageAndTs(event, logEventMap);
+        addMessageAndTimestamp(event, logEventMap);


Just a rename - I felt that Ts was not really clear enough

christopher-gilbert · 2023-03-11T12:19:47Z

...tation/apache-log4j-1/src/main/java/com/nr/agent/instrumentation/log4j1/LoggingEventMap.java

-            Object key = entry.getKey();
-            Object value = entry.getValue();
+    private static void addMdc(Map<?, ?> mdc, Map<LogAttributeKey, Object> logEventMap) {
+        mdc.forEach((key, value) -> {


I note that Java 8 is the earliest supported version for current releases of the agent, so just using slightly more succinct syntax. Same change in all the logging instrumentation.

christopher-gilbert · 2023-03-11T12:21:54Z

...tation/apache-log4j-1/src/main/java/com/nr/agent/instrumentation/log4j1/LoggingEventMap.java

-        for (Map.Entry<?, ?> entry : mdc.entrySet()) {
-            Object key = entry.getKey();
-            Object value = entry.getValue();
+    private static void addMdc(Map<?, ?> mdc, Map<LogAttributeKey, Object> logEventMap) {


@meiao log4j-2 instrumentation code uses the name logEventMap which is clearer, so have applied that name consistently across the logging instrumentation.

christopher-gilbert · 2023-03-11T12:31:33Z

newrelic-agent/src/main/java/com/newrelic/agent/config/AgentConfigImpl.java

@@ -551,7 +551,7 @@ private TransactionNamingScheme parseTransactionNamingMode() {
     */
    private void checkHighSecurityPropsInFlattened(Map<String, Object> flattenedProps) {
        if (highSecurity && !flattenedProps.isEmpty()) {
-            flattenedProps.put("transaction_tracer.record_sql", transactionTracerConfig.getRecordSql());
+            flattenedProps.put("transaction_tracer.record_sql", new ParsedConfigValue(transactionTracerConfig.getRecordSql()));


ParsedConfigValue is the type I introduced to deal with properties that can represent different data types. In this case it is just wrapping the existing String

christopher-gilbert · 2023-03-11T16:39:52Z

newrelic-agent/src/main/java/com/newrelic/agent/config/AgentConfigImpl.java

        if (value == null) {
            return defaultValue;
        } else if (value instanceof ServerProp) {
            value = ((ServerProp) value).getValue();
            return castValue(path, value, defaultValue);
-        } else if (value instanceof String && defaultValue instanceof Boolean) {


This coercion to Boolean or Integer has been shifted out to the ParsedConfigValue class

christopher-gilbert · 2023-03-11T16:50:48Z

newrelic-agent/src/main/java/com/newrelic/agent/config/BaseConfig.java

+     * If the String format cannot be parsed as a sequence of Map entries, or if the consumer throws a
+     * {@link ParseException} then a {@link ParseException} will be thrown.
+     */
+    public static void parseMapEntriesFromString(String mapAsString, MapParsingBiConsumer<String, String> parsedEntryHandler) throws


This was in LabelsConfigImpl - I've shifted it up to this base class as a static method to align with the existing similar methods in this class eg getUniqueStringsFromString.

This means that this logic is available to the flattened config access as well as the original typesafe style Configs (LabelsConfig etc)

However, I have taken out labels config specific validation, and just allowed it and other users, to pass in their own handler to do what they want with the parsed map entries.

christopher-gilbert · 2023-03-11T16:54:59Z

newrelic-agent/src/main/java/com/newrelic/agent/config/ParsedConfigValue.java

+    private Boolean valueAsBoolean;
+    private Integer valueAsInteger;
+
+    public ParsedConfigValue(Object value) {


Really this is just pulling together existing functionality into one place. Basic idea is that the value is interpreted in as many ways as possible at time of construction, and can then be retrieved as any of the valid types by specifying a default of the required type

christopher-gilbert · 2023-03-11T16:55:30Z

newrelic-agent/src/test/java/com/newrelic/agent/config/LabelsConfigImplTest.java

@@ -140,6 +140,9 @@ public void testInvalidString() {

        config = new LabelsConfigImpl("foo:bar;;many:delimiters");
        assertTrue(config.getLabels().isEmpty());
+
+        config = new LabelsConfigImpl("foo:bar;empty:    ");


Just a missing test case added in

kford-newrelic · 2023-03-27T13:48:15Z

@christopher-gilbert thanks for this PR! We intend to review/test this during our Apr-Jun quarter's roadmap work

kford-newrelic · 2023-04-13T23:45:51Z

@chris-gilbert-2 our engineers thought your suggestion was a good one - but they felt that it should be considered for standardization in our agent specification (a good thing; and subsequently implemented across all our APM agents). We're going to trigger those discussions during this Apr-Jun quarter but because that takes time to get buy in from other engineers, we'll have to keep your PR on hold for the moment.

jasonjkeller · 2023-06-12T17:36:12Z

Closing PR. See explanation here: #1104 (comment)

chris-gilbert-2 added 6 commits December 27, 2022 14:54

NEWRELIC-5922 - Extract map parsing out of LabelsConfigImpl so that the

c054d54

general parsing of Strings to Maps does not have labels specific validation applied. Applying tag feature to Logback. General tidy up.

NEWRELIC-5922 - Merge branch 'main' into feature/add-tags-to-forwarde…

7c33a23

…d-logs Fix conflicts by adding the instrumentation attribute to the event map of agents that had been updated on feature branch

NEWRELIC-5922 - add tags to Jboss logging agent

1e86196

Add tags, and apply tidy up that was introduced in a previous PR and has now been applied to all logging agents for consistency.

Merge branch 'main' into feature/add-tags-to-forwarded-logs

06a8eb0

NEWRELIC-5922 - small fixes following merges from main branch

bec9ff3

christopher-gilbert commented Mar 11, 2023

View reviewed changes

christopher-gilbert marked this pull request as ready for review March 11, 2023 16:57

christopher-gilbert mentioned this pull request Mar 11, 2023

Java agent: Include configured labels as tags in forwarded application log events #1104

Closed

kford-newrelic added the on-roadmap Issue has been added to our product roadmap and will be worked in the coming quarter label Mar 16, 2023

jasonjkeller closed this Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add tags to forwarded logs #1177

Feature/add tags to forwarded logs #1177

christopher-gilbert commented Mar 10, 2023

CLAassistant commented Mar 10, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

christopher-gilbert Mar 11, 2023

kford-newrelic commented Mar 27, 2023

kford-newrelic commented Apr 13, 2023

jasonjkeller commented Jun 12, 2023

Feature/add tags to forwarded logs #1177

Feature/add tags to forwarded logs #1177

Conversation

christopher-gilbert commented Mar 10, 2023

CLAassistant commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kford-newrelic commented Mar 27, 2023

kford-newrelic commented Apr 13, 2023

jasonjkeller commented Jun 12, 2023