Expand exception logs #168

pavolloffay · 2017-05-04T10:23:33Z

No description provided.

pavolloffay · 2017-05-04T10:29:17Z

jaeger-core/src/main/java/com/uber/jaeger/Tracer.java

@@ -389,6 +397,17 @@ public Builder withZipkinSharedRpcSpan() {
      return this;
    }

+    /**
+     * If the exception is logged and 'message', 'stack' or 'error.kind' is missing. Tracer will


message, error.kind could be done automatically, and stack could be optional for perf reasons.

Nit: comma after "missing"

codecov-io · 2017-05-04T15:00:39Z

Codecov Report

Merging #168 into master will increase coverage by 0.15%.
The diff coverage is 95.65%.

@@             Coverage Diff              @@
##             master     #168      +/-   ##
============================================
+ Coverage     84.28%   84.43%   +0.15%     
- Complexity      575      583       +8     
============================================
  Files            92       92              
  Lines          2246     2268      +22     
  Branches        260      266       +6     
============================================
+ Hits           1893     1915      +22     
  Misses          251      251              
  Partials        102      102

Impacted Files	Coverage Δ	Complexity Δ
...ger-core/src/main/java/com/uber/jaeger/Tracer.java	`87.06% <100%> (+0.22%)`	`21 <2> (+1)`	⬆️
...aeger-core/src/main/java/com/uber/jaeger/Span.java	`84.67% <94.44%> (+2.6%)`	`46 <5> (+7)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fa8d12...b86f307. Read the comment docs.

black-adder

Tests to come?

black-adder · 2017-05-04T14:57:19Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

+   * @return logged fields
+   */
+  private static Map<String, Object> exceptionLogs(Map<String, ?> fields) {
+    Object ex = fields.get("error.object");


are "error.object", "stack", "message", and "error.kind" defined in opentracing Java? If not, should we make them public consts for jaeger-client-java?

They are defined in the spec, but not in API artifact, I will open PR to ot-java.

black-adder · 2017-05-04T15:01:32Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

+          if (!errorFields.isEmpty()) {
+            errorFields.putAll(fields);
+          }
+          fields = errorFields;


it looks like if the fields don't contain "error.object", we throw out all the fields the user passed in?

good catch, this should be inside, the if above

black-adder · 2017-05-04T15:03:47Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

        logs.add(new LogData(timestampMicroseconds, fields));
      }
      return this;
    }
  }

+


nit: double newline uncessecary

pavolloffay · 2017-05-05T08:38:52Z

PR updated, added some tests

black-adder · 2017-05-05T14:08:47Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

+          Map<String, Object> errorFields = exceptionLogs(fields);
+          if (!errorFields.isEmpty()) {
+            errorFields.putAll(fields);
+            fields = errorFields;


I'm not overly familiar with certain Java things, but given that we do errorFields.putAll(fields) above, is it still necessary for fields = errorFields;?

It is necessary because of line 237.

ohhh, putAll puts fields into errorFields and not the other way round (duhhh).

Given that, this means if the user specifically added a "error.object", "stack", "message", and "error.kind", that will overwrite the k,v in errorFields? sounds right.

Tracer does not change explicitly logged items ( "error.object", "stack", "message" ..). It only derives new fields if are not logged.

black-adder · 2017-05-05T14:12:25Z

LGTM, I wonder if we should wait until the tags are specified in ot-java before landing this though.

jpkrohling · 2018-02-13T15:23:18Z

Is this still relevant ?

pavolloffay · 2018-02-16T16:53:11Z

Yes I will rebase it

pavolloffay · 2018-02-22T17:18:14Z

Rebased, it can be reviewed. The last review comment was to wait for Fields to be defined in OT. They have been added in 0.31.0

yurishkuro · 2018-02-22T18:39:34Z

jaeger-core/src/main/java/com/uber/jaeger/Span.java

@@ -228,6 +231,13 @@ public Span log(long timestampMicroseconds, Map<String, ?> fields) {
        return this;
      }
      if (context.isSampled()) {
+        if (tracer.isExpandExceptionLogs()) {
+          Map<String, Object> errorFields = exceptionLogs(fields);


minor point, but couldn't we move all the logic into exceptionLogs() so that here we only have

fields = exceptionLogs(fields);

?

Internally it could do Map<String, Object> errorFields = new HashMap<String, Object>(fields);

I have changed it to ^^. I will merge or green

pavolloffay · 2018-02-23T09:13:18Z

Coverage is odd, there are no changes to that class/tests in this PR.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay commented May 4, 2017

View reviewed changes

pavolloffay force-pushed the stacktrace branch from 377b498 to 17ec75f Compare May 4, 2017 14:40

black-adder reviewed May 4, 2017

View reviewed changes

pavolloffay force-pushed the stacktrace branch from 8a8335c to 2e7f839 Compare May 5, 2017 08:38

pavolloffay force-pushed the stacktrace branch from 2e7f839 to badf918 Compare May 5, 2017 08:39

black-adder reviewed May 5, 2017

View reviewed changes

pavolloffay force-pushed the stacktrace branch from badf918 to 7ebb302 Compare May 5, 2017 16:35

pavolloffay force-pushed the stacktrace branch from be55b5a to 08c151b Compare February 22, 2018 17:16

ghost assigned pavolloffay Feb 22, 2018

ghost added the review label Feb 22, 2018

pavolloffay force-pushed the stacktrace branch from 08c151b to 0d6a303 Compare February 22, 2018 17:17

yurishkuro approved these changes Feb 22, 2018

View reviewed changes

pavolloffay added 2 commits February 26, 2018 13:40

Expand exception logs

75679c6

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix review comments

adefa74

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the stacktrace branch from a567ade to adefa74 Compare February 26, 2018 12:40

increase coverage

c8dd269

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the stacktrace branch from 5fc098b to c8dd269 Compare February 26, 2018 12:52

pavolloffay added 3 commits February 26, 2018 14:08

Increase coverage 2

50a0e08

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Increase coverage 3

b1ac3cb

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

fix style

b86f307

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay merged commit e152db4 into jaegertracing:master Feb 26, 2018

ghost removed the review label Feb 26, 2018

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand exception logs #168

Expand exception logs #168

pavolloffay commented May 4, 2017

pavolloffay May 4, 2017

yurishkuro May 5, 2017

codecov-io commented May 4, 2017 •

edited by codecov bot

black-adder left a comment

black-adder May 4, 2017

pavolloffay May 4, 2017

black-adder May 4, 2017

pavolloffay May 4, 2017

black-adder May 4, 2017

pavolloffay commented May 5, 2017

black-adder May 5, 2017

pavolloffay May 5, 2017

black-adder May 5, 2017

pavolloffay May 5, 2017

black-adder commented May 5, 2017

jpkrohling commented Feb 13, 2018

pavolloffay commented Feb 16, 2018

pavolloffay commented Feb 22, 2018

yurishkuro Feb 22, 2018

pavolloffay Feb 23, 2018

pavolloffay commented Feb 23, 2018 •

edited

Expand exception logs #168

Expand exception logs #168

Conversation

pavolloffay commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 4, 2017 • edited by codecov bot

Codecov Report

black-adder left a 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

pavolloffay commented May 5, 2017

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

black-adder commented May 5, 2017

jpkrohling commented Feb 13, 2018

pavolloffay commented Feb 16, 2018

pavolloffay commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Feb 23, 2018 • edited

codecov-io commented May 4, 2017 •

edited by codecov bot

pavolloffay commented Feb 23, 2018 •

edited