Conversation
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
interesting, I didn't do this manually.
There was a problem hiding this comment.
you need to fix your IDE formatter. I think readme has links to IntelliJ settings
| if (logs == null) { | ||
| this.logs = new ArrayList<>(); | ||
| } | ||
| for (Map.Entry<String, ?> kv : fields.entrySet()) { |
There was a problem hiding this comment.
The intended use for jaeger log fields are that each log can be associated with multiple tags. Here, I'm creating a new log for each tag. In the JaegerThriftConverter I can add a special case for when the value is a map and generate multiple tags for the same log.
There was a problem hiding this comment.
-1. A log record is a timestamp + a set of fields.
There was a problem hiding this comment.
also, we don't want to do the conversion of fields here, we just want to stash the map somewhere and only do conversion when the span is reported.
Codecov Report
@@ Coverage Diff @@
## master #131 +/- ##
============================================
+ Coverage 81.35% 81.46% +0.11%
- Complexity 512 524 +12
============================================
Files 90 90
Lines 1936 1964 +28
Branches 189 196 +7
============================================
+ Hits 1575 1600 +25
- Misses 270 271 +1
- Partials 91 93 +2
Continue to review full report at Codecov.
|
build.gradle
Outdated
|
|
||
|
|
||
| ext.opentracingVersion = '0.15.0' | ||
| ext.opentracingVersion = '0.20.9' |
| if (context.isSampled()) { | ||
| if (logs == null) { | ||
| this.logs = new ArrayList<>(); | ||
| } |
There was a problem hiding this comment.
Self-inflicted inefficiency here, you reuse (String, Object) signature and then are forced to check for instanceof Map. Not to mention that there may be a payload that's a map.
I suggest simply having another constructor for LogData that stores Map<String, ?> fields proper.
| if (logData.getFields() != null) { | ||
| jLog.setFields(buildTags(logData.getFields())); | ||
| } else { | ||
| final Tag tag = buildTag(logData.getMessage(), logData.getPayload()); |
There was a problem hiding this comment.
I think this is incorrect. Should be "event": message, "payload": payload (and drop either if null)
| } | ||
|
|
||
| @Override | ||
| public Span log(long instantMicroseconds, String message, /* @Nullable */ Object payload) { |
There was a problem hiding this comment.
given that this function is deprecated, I'm not going to do a check here to see if the payload is a map.
There was a problem hiding this comment.
I wasn't suggesting it. Payload as map should be stringified as any other object.
|
|
||
| public Object getPayload() { | ||
| return payload; | ||
| LogData(long time, Map fields) { |
|
@black-adder did you cover the new methods? why did coverage go down? |
|
I'm gonna go ahead and land this, the code that isnt covered wasn't covered before but I moved it one line down and all hell broke loose. Will add followup PR to cover the rest. |
4 participants
No description provided.