Add support for service.environment #184

odin568 · 2022-04-29T06:32:47Z

As an extension of #168
We are hosting different environments of our applications, i.e. dev, test and of course prod.
In order to distinguish logs, we set the service.environment field. With this change it is easier to achieve.
Please review and feel free to merge.

cla-checker-service · 2022-04-29T06:32:51Z

💚 CLA has been signed

apmmachine · 2022-04-29T06:36:26Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-05-02T08:07:44.403+0000
Duration: 7 min 47 sec

Test stats 🧪

Test	Results
Failed	0
Passed	243
Skipped	0
Total	243

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

felixbarny · 2022-04-29T08:10:53Z

@odin568 thanks for your contribution! This makes a lot of sense to me.
Please make sure to sign the CLA so that we are able to merge the PR.

odin568 · 2022-04-29T09:44:32Z

@odin568 thanks for your contribution! This makes a lot of sense to me. Please make sure to sign the CLA so that we are able to merge the PR.

Hi,
yes I tried. Seems to be more complex than the change. Not sure if I submitted multiple times, did not get any mail, etc. so far.
Sorry for inconvenience then ;)

felixbarny · 2022-04-29T10:58:15Z

Please make sure that the email address you're using to sign the CLA is also registered in your GitHub settings (https://github.com/settings/emails).

odin568 · 2022-04-29T11:19:29Z

Please make sure that the email address you're using to sign the CLA is also registered in your GitHub settings (https://github.com/settings/emails).

Valid Point. I re-signed it, no Mail so far.

felixbarny · 2022-04-29T11:24:41Z

I can see you in the system with two different email addresses now. However, the mail used in your git config (see https://github.com/elastic/ecs-logging-java/pull/184.patch) is a different one.

odin568 · 2022-04-29T11:48:35Z

I can see you in the system with two different email addresses now. However, the mail used in your git config (see https://github.com/elastic/ecs-logging-java/pull/184.patch) is a different one.

Allright, now you should have three :)
Sorry, too many different workstations

eyalkoren

Looks great, thanks!
A couple of minor space removals, plus one request to make sure we do not add platform-specific assertion to tests

jul-ecs-formatter/src/main/java/co/elastic/logging/jul/EcsFormatter.java

log4j-ecs-layout/src/main/java/co/elastic/logging/log4j/EcsLayout.java

log4j2-ecs-layout/src/main/java/co/elastic/logging/log4j2/EcsLayout.java

eyalkoren · 2022-05-01T12:17:34Z

...2-ecs-layout/src/test/java/co/elastic/logging/log4j2/EcsLayoutWithStackTraceAsArrayTest.java

@@ -50,7 +50,7 @@ void testLogException() throws Exception {
        assertThat(log.get("error.stack_trace").isArray()).isTrue();
        ArrayNode arrayNode = (ArrayNode) log.get("error.stack_trace");
        assertThat(arrayNode.size()).isEqualTo(4);
-        assertThat(arrayNode.get(0).textValue()).isEqualTo("java.lang.NumberFormatException: For input string: \"NOT_AN_INT\"");
+        assertThat(arrayNode.get(0).textValue()).isEqualTo("java.lang.NumberFormatException: For input string: \"NOT_AN_INT\"\r");


Looks platform-specific. Does this fail for you without that?
If so, better to change to use startsWith or regex comparison instead

Test on CI (Ubuntu 18) fails with:

[2022-05-01T12:33:28.698Z] Failed tests: [2022-05-01T12:33:28.698Z] EcsLayoutWithStackTraceAsArrayTest.testLogException:53 [2022-05-01T12:33:28.698Z] Expecting: [2022-05-01T12:33:28.698Z] <"java.lang.NumberFormatException: For input string: "NOT_AN_INT""> [2022-05-01T12:33:28.698Z] to be equal to: [2022-05-01T12:33:28.698Z] <"java.lang.NumberFormatException: For input string: "NOT_AN_INT" "> [2022-05-01T12:33:28.698Z] but was not.

Yes, it fails on Windows otherwise. Added a trim before, should be platform-independent then.

I don't think this is an issue in the tests but a bug here:

ecs-logging-java/ecs-logging-core/src/main/java/co/elastic/logging/EcsJsonSerializer.java

Line 39 in b09574f

private static final Pattern NEW_LINE_PATTERN = Pattern.compile("\\n");

It always splits on \n instead of depending on the platform.

You are right. That's why I got \r at the end. Wondered about mac style... but is clear if it splitted by \n.
Generalized it in latest commit and removed the .trim() again.

...2-ecs-layout/src/test/java/co/elastic/logging/log4j2/EcsLayoutWithStackTraceAsArrayTest.java

logback-ecs-encoder/src/main/java/co/elastic/logging/logback/EcsEncoder.java

eyalkoren · 2022-05-01T12:24:13Z

/test

odin568 · 2022-05-02T06:43:36Z

I applied changes you mentioned. Thanks for feedback!

\r\n was splitted only by \n which is problematic on windows. Generalized to use \R which matches all combinations of line breaks.

felixbarny · 2022-05-02T08:02:42Z

ecs-logging-core/src/main/java/co/elastic/logging/EcsJsonSerializer.java

@@ -36,7 +36,7 @@ public class EcsJsonSerializer {
    private static final TimestampSerializer TIMESTAMP_SERIALIZER = new TimestampSerializer();
    private static final ThreadLocal<StringBuilder> messageStringBuilder = new ThreadLocal<StringBuilder>();
    private static final String NEW_LINE = System.getProperty("line.separator");
-    private static final Pattern NEW_LINE_PATTERN = Pattern.compile("\\n");
+    private static final Pattern NEW_LINE_PATTERN = Pattern.compile("\\R");


\R has been introduced in Java 8 so this won't work with older Java versions which this library is compatible with.

You could either use the equivalent of \R (\u000D\u000A|[\u000A\u000B\u000C\u000D\u0085\u2028\u2029]) or the more simple alternative "\\r?\\n" which should cover most relevant cases (ignores Mac OS 9 style \r, though).

ah okay, was not aware that < Java 8 is supported still. Yes then let's make it right. The pattern I like as it is simple to read is \r\n|\n|\r - covers all cases for a single newline. Will update

felixbarny

LGTM, thanks!

eyalkoren

Thanks!

Add service.Environment as dedicated field

0b1fb5d

github-actions bot added agent-java community Issues and PRs created by the community triage Issues and PRs that need to be triaged labels Apr 29, 2022

felixbarny changed the title ~~Add service.Environment as dedicated field~~ Add support for service.environment Apr 29, 2022

felixbarny mentioned this pull request Apr 29, 2022

Add service.environment and service.version fields to the spec elastic/ecs-logging#70

Closed

felixbarny requested a review from eyalkoren April 29, 2022 15:27

eyalkoren reviewed May 1, 2022

View reviewed changes

Apply review changes

41da1c4

Fix root cause of testing issue:

bf486bf

\r\n was splitted only by \n which is problematic on windows. Generalized to use \R which matches all combinations of line breaks.

felixbarny reviewed May 2, 2022

View reviewed changes

Make pattern compatible with < Java 8

b5fd699

felixbarny approved these changes May 2, 2022

View reviewed changes

eyalkoren approved these changes May 2, 2022

View reviewed changes

eyalkoren merged commit 9062b71 into elastic:main May 2, 2022

felixbarny mentioned this pull request May 3, 2022

Support more ECS service fields #129

Closed

trentm mentioned this pull request May 10, 2022

add support for service.version and service.environment with apmInstrumentation elastic/ecs-logging-nodejs#121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for service.environment #184

Add support for service.environment #184

odin568 commented Apr 29, 2022 •

edited by zube bot

Loading

cla-checker-service bot commented Apr 29, 2022 •

edited

Loading

apmmachine commented Apr 29, 2022 •

edited

Loading

Build stats

Test stats 🧪

felixbarny commented Apr 29, 2022

odin568 commented Apr 29, 2022

felixbarny commented Apr 29, 2022

odin568 commented Apr 29, 2022

felixbarny commented Apr 29, 2022

odin568 commented Apr 29, 2022

eyalkoren left a comment •

edited

Loading

eyalkoren May 1, 2022

eyalkoren May 1, 2022

odin568 May 2, 2022

felixbarny May 2, 2022

odin568 May 2, 2022 •

edited

Loading

eyalkoren commented May 1, 2022

odin568 commented May 2, 2022

felixbarny May 2, 2022

odin568 May 2, 2022 •

edited

Loading

felixbarny left a comment

eyalkoren left a comment

Add support for service.environment #184

Add support for service.environment #184

Conversation

odin568 commented Apr 29, 2022 • edited by zube bot Loading

cla-checker-service bot commented Apr 29, 2022 • edited Loading

apmmachine commented Apr 29, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

felixbarny commented Apr 29, 2022

odin568 commented Apr 29, 2022

felixbarny commented Apr 29, 2022

odin568 commented Apr 29, 2022

felixbarny commented Apr 29, 2022

odin568 commented Apr 29, 2022

eyalkoren left a comment • edited Loading

Choose a reason for hiding this comment

eyalkoren May 1, 2022

Choose a reason for hiding this comment

eyalkoren May 1, 2022

Choose a reason for hiding this comment

odin568 May 2, 2022

Choose a reason for hiding this comment

felixbarny May 2, 2022

Choose a reason for hiding this comment

odin568 May 2, 2022 • edited Loading

Choose a reason for hiding this comment

eyalkoren commented May 1, 2022

odin568 commented May 2, 2022

felixbarny May 2, 2022

Choose a reason for hiding this comment

odin568 May 2, 2022 • edited Loading

Choose a reason for hiding this comment

felixbarny left a comment

Choose a reason for hiding this comment

eyalkoren left a comment

Choose a reason for hiding this comment

odin568 commented Apr 29, 2022 •

edited by zube bot

Loading

cla-checker-service bot commented Apr 29, 2022 •

edited

Loading

apmmachine commented Apr 29, 2022 •

edited

Loading

eyalkoren left a comment •

edited

Loading

odin568 May 2, 2022 •

edited

Loading

odin568 May 2, 2022 •

edited

Loading