Add logging API to the SDK #159

jkwatson · 2020-05-08T22:44:01Z

This addition to the SDK will be useful for non-typical logging usages, such as reporting OpenTelemetry Span Events as log entries, in context. It's not intended as an API to be used to implement high-throughput logging systems. That use-case should be reserved for log forwarder implementations.

jeffalder

Even though it's nice to have the symmetry for L as we do for M, E, and T, I don't think this is the right approach. It doesn't seem useful not being integrated with any logging framework, and I think it looks at "logs as telemetry" instead of "application logs as one set of logs in an application stack".

When we implemented logs-in-context, there was a lot of debate about whether the agent should just send the log messages or we should rely on something else to do it. Matt Culmone, one of the Log PMs, was adamant that the application should not be posting logs to an HTTP endpoint. There are existing solutions for this, like log forwarders, that do a far better job batching, scanning, rewinding, etc across multiple services and multiple sources than we ever could. If the user wants to consume any other type of logging, like access logs or nginx logs or non-Java logs, they would still need a log forwarder.

Before shipping this implementation, I'd encourage you to reach out to Matt or Rebecca Holzschuh to understand what they'd recommend.

gradle.properties

telemetry_core/src/main/java/com/newrelic/telemetry/SenderConfiguration.java

telemetry_core/src/main/java/com/newrelic/telemetry/SpanBatchSenderFactory.java

telemetry_core/src/main/java/com/newrelic/telemetry/logs/Log.java

jeffalder · 2020-05-09T19:43:53Z

telemetry_core/src/main/java/com/newrelic/telemetry/logs/LogBatchSender.java

+    logger.debug(
+        "Sending a log batch (number of logs: {}) to the New Relic log ingest endpoint)",
+        batch.size());
+    String json = marshaller.toJson(batch);


We need to rethink the sender if we're going to implement this as-is. If the sender does literally any logging you could quickly drown yourself in recursive logs without careful configuration. Think about audit logging:

There's an http post.

There's a log message with the contents of the post.

There's an http post with the log message with the contents of the post.

There's a log message with the contents of the post with the log message with the contents of the post.

There is no expectation that this code would be used as the API for a full-fledged logging implementation, so I don't think this is a concern.

jkwatson · 2020-05-11T15:14:01Z

Even though it's nice to have the symmetry for L as we do for M, E, and T, I don't think this is the right approach. It doesn't seem useful not being integrated with any logging framework, and I think it looks at "logs as telemetry" instead of "application logs as one set of logs in an application stack".

When we implemented logs-in-context, there was a lot of debate about whether the agent should just send the log messages or we should rely on something else to do it. Matt Culmone, one of the Log PMs, was adamant that the application should not be posting logs to an HTTP endpoint. There are existing solutions for this, like log forwarders, that do a far better job batching, scanning, rewinding, etc across multiple services and multiple sources than we ever could. If the user wants to consume any other type of logging, like access logs or nginx logs or non-Java logs, they would still need a log forwarder.

Before shipping this implementation, I'd encourage you to reach out to Matt or Rebecca Holzschuh to understand what they'd recommend.

Hi Jeff. My use-case for this is not acting as the primary API for logging implementations. This is to support some log-like features of OpenTelemetry. In particular, the events that can be attached to Spans are being used to log information about the processing of a given span. I'd like to experiment with writing them as log entries using the logging API. I discussed this with Rebecca and she agreed.

jkwatson · 2020-05-26T19:21:45Z

@jeffalder note, my use-case for this is supporting the "embedded" log model being discussed here: open-telemetry/opentelemetry-specification#622

justinfoote

Overall, I'm in favor of the concept. If we're providing an HTTP endpoint to send logs to, it only makes sense that we'd include functionality to use that endpoint in our telemetry SDKs.

Before we go through with this, I'd like to see the exporter specs and the telemetry SDK specs updated to match.

justinfoote · 2020-05-26T20:41:24Z

telemetry_core/src/main/java/com/newrelic/telemetry/logs/Log.java

+      ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+      e.printStackTrace(new PrintStream(bytes));
+      this.message = bytes.toString();
+      this.logType = "stackTrace";


Is this a convention?

If so, it doesn't align with our other logging error semantics, here: https://github.com/newrelic/newrelic-exporter-specs/tree/master/logging#errors

ooh! I totally forgot there were semantics already designed for some of this stuff. Will address!

jkwatson · 2020-05-26T22:13:44Z

Before we go through with this, I'd like to see the exporter specs and the telemetry SDK specs updated to match.

Chicken and egg! I don't think we'd want to update the exporter specs until we've actually got an exporter using this and it's the officially sanctioned way to send span events (or whatever). Telemetry SDK, though, yes. Should we get this at least merged before we spend time writing specs for it, pre-release?

telemetry_core/src/main/java/com/newrelic/telemetry/logs/json/LogJsonCommonBlockWriter.java

jeffalder · 2020-05-27T16:11:11Z

@jkwatson

Before it became real, much testing and documentation would need to be added.

I don't see any new tests here, and it looks like this is ready to merge.

There's so much code cloned from the span side because the API shapes are so similar. Can you explain why they're cloned?

jkwatson · 2020-05-27T16:31:50Z

@jkwatson

Before it became real, much testing and documentation would need to be added.

I don't see any new tests here, and it looks like this is ready to merge.

There's so much code cloned from the span side because the API shapes are so similar. Can you explain why they're cloned?

I need to add tests; just realized that there are a lot missing still. I'll be doing that ASAP.

The code is cloned because I was moving fast to get something working. After this gets merged, I think that's definitely something to figure out how to unify.

Co-authored-by: jeffalder <49817386+jeffalder@users.noreply.github.com>

…LogJsonCommonBlockWriter.java Co-authored-by: jeffalder <49817386+jeffalder@users.noreply.github.com>

add a log integration test

jkwatson · 2020-05-27T17:56:54Z

ok, added tests for json generation, and an integration test. Also got the attribute names in line with the exporter specs for logs.

jeffalder · 2020-05-28T15:25:09Z

The code is cloned because I was moving fast to get something working. After this gets merged, I think that's definitely something to figure out how to unify.

We already have a backlog of 16 tickets. Are you suggesting just tacking on more to the backlog? That is an issue that will never get done because it's tedious housekeeping. I feel like this is how tech debt becomes overwhelming, and we have reactions like "if we had just done this at the time ..."

I feel we have a responsibility to maintain maximally clean code. Unless there's roadmap pressure to ship this that I'm not aware of?

jkwatson · 2020-05-28T15:34:17Z

@jeffalder do you think that this PR should also include significant refactoring of unrelated code? Do you have specific examples of where you want things refactored? I feel like refactoring should happen in a separate PR from one that adds new functionality.

jkwatson · 2020-05-28T16:14:07Z

Before we go through with this, I'd like to see the exporter specs and the telemetry SDK specs updated to match.

Chicken and egg! I don't think we'd want to update the exporter specs until we've actually got an exporter using this and it's the officially sanctioned way to send span events (or whatever). Telemetry SDK, though, yes. Should we get this at least merged before we spend time writing specs for it, pre-release?

@justinfoote newrelic/newrelic-telemetry-sdk-specs#16

jeffalder suggested changes May 9, 2020

View reviewed changes

jeffalder added this to In progress in Java Engineering Board May 13, 2020

jkwatson force-pushed the add_logs branch from 6d6c37c to 3f89c7b Compare May 20, 2020 20:29

jkwatson marked this pull request as ready for review May 20, 2020 20:29

justinfoote reviewed May 26, 2020

View reviewed changes

jeffalder reviewed May 27, 2020

View reviewed changes

telemetry_core/src/main/java/com/newrelic/telemetry/logs/json/LogJsonCommonBlockWriter.java Outdated Show resolved Hide resolved

jwatson and others added 8 commits May 27, 2020 10:54

first steps to adding logs

094162a

Update telemetry_core/src/main/java/com/newrelic/telemetry/logs/Log.java

6fc8569

Co-authored-by: jeffalder <49817386+jeffalder@users.noreply.github.com>

Add javadoc, and support in the TelemetryClient for sending log entries.

97a93b3

rebased and fixed a broken test

5c81598

fix javadoc and clean up some warnings

9480fa5

Update telemetry_core/src/main/java/com/newrelic/telemetry/logs/json/…

a4831b3

…LogJsonCommonBlockWriter.java Co-authored-by: jeffalder <49817386+jeffalder@users.noreply.github.com>

make the attributes match the semantic conventions

dce573d

add a log integration test

Add unit tests for log json generation

49248c8

jkwatson force-pushed the add_logs branch from 6f54284 to 49248c8 Compare May 27, 2020 17:54

jkwatson mentioned this pull request May 28, 2020

Add a specification for the logging API newrelic/newrelic-telemetry-sdk-specs#16

Merged

jeffalder approved these changes May 28, 2020

View reviewed changes

jeffalder merged commit 9e07000 into master May 28, 2020

Java Engineering Board automation moved this from In progress to Done May 28, 2020

jeffalder deleted the add_logs branch May 28, 2020 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging API to the SDK #159

Add logging API to the SDK #159

jkwatson commented May 8, 2020 •

edited

jeffalder left a comment

jeffalder May 9, 2020

jkwatson May 11, 2020

jkwatson commented May 11, 2020

jkwatson commented May 26, 2020

justinfoote left a comment

justinfoote May 26, 2020

jkwatson May 26, 2020

jkwatson commented May 26, 2020

jeffalder commented May 27, 2020 •

edited

jkwatson commented May 27, 2020

jkwatson commented May 27, 2020

jeffalder commented May 28, 2020

jkwatson commented May 28, 2020

jkwatson commented May 28, 2020

Add logging API to the SDK #159

Add logging API to the SDK #159

Conversation

jkwatson commented May 8, 2020 • edited

jeffalder left a comment

Choose a reason for hiding this comment

jeffalder May 9, 2020

Choose a reason for hiding this comment

jkwatson May 11, 2020

Choose a reason for hiding this comment

jkwatson commented May 11, 2020

jkwatson commented May 26, 2020

justinfoote left a comment

Choose a reason for hiding this comment

justinfoote May 26, 2020

Choose a reason for hiding this comment

jkwatson May 26, 2020

Choose a reason for hiding this comment

jkwatson commented May 26, 2020

jeffalder commented May 27, 2020 • edited

jkwatson commented May 27, 2020

jkwatson commented May 27, 2020

jeffalder commented May 28, 2020

jkwatson commented May 28, 2020

jkwatson commented May 28, 2020

jkwatson commented May 8, 2020 •

edited

jeffalder commented May 27, 2020 •

edited