Initial Commit #1
Conversation
build.gradle
Outdated
|
|
||
| dependencies { | ||
| compile group: 'io.zipkin.zipkin2', name: 'zipkin', version: '2.8.3' | ||
| compile group: 'io.zipkin.reporter2', name: 'zipkin-reporter', version: '2.6.0' |
There was a problem hiding this comment.
you can use the zipkin reporter bom to avoid tracking two versions https://github.com/openzipkin/zipkin-reporter-java#version-alignments looks simiar to this https://github.com/hyleung/ratpack-zipkin/pull/60/files
build.gradle
Outdated
| dependencies { | ||
| compile group: 'io.zipkin.zipkin2', name: 'zipkin', version: '2.8.3' | ||
| compile group: 'io.zipkin.reporter2', name: 'zipkin-reporter', version: '2.6.0' | ||
| compile group: 'org.msgpack', name: 'jackson-dataformat-msgpack', version: '0.8.16' |
There was a problem hiding this comment.
consider shading this to avoid library conflicts (since this is a 0.x library)
There was a problem hiding this comment.
not sure this is worth the effort. skipping for now.
build.gradle
Outdated
| compile group: 'io.zipkin.reporter2', name: 'zipkin-reporter', version: '2.6.0' | ||
| compile group: 'org.msgpack', name: 'jackson-dataformat-msgpack', version: '0.8.16' | ||
|
|
||
| compile group: 'org.slf4j', name: 'slf4j-api', version: '1.7.0' |
There was a problem hiding this comment.
consider killing this so that folks using log4j2 or otherwise don't get slf4j warnings only on account of this code. This implies using JUL, which has well understood limitations.
| private static final ObjectMapper objectMapper = new ObjectMapper(new MessagePackFactory()); | ||
|
|
||
| public DDApi(final String host, final int port) { | ||
| this(host, port, traceEndpointAvailable("http://" + host + ":" + port + TRACES_ENDPOINT_V4)); |
There was a problem hiding this comment.
careful about I/O in constructors. usually it is nicer to be lazy about this, for example initializing it when the reporter thread is or on first call to flush (if external flushing is supported)
There was a problem hiding this comment.
the tradeoff is the endpoint URL can't be final. I kinda like the final... I understand it's less ideal to do this, but is it really a big deal?
There was a problem hiding this comment.
yep it can crash apps during wiring just because zipkin isn't available. it is a well-known anti-pattern
There was a problem hiding this comment.
incidentally, I can't find the anti-pattern note as it was years ago.. this one is a similar notice. https://github.com/google/guice/wiki/BeCarefulAboutIoInProviders
basically if you have something like this in spring, the whole app will crash. that's the part that is a big deal. That's why in zipkin we have this "Component" interface which defers i/o but lets things get wired together.
Anyway, you can still do this, just make a disclaimer about it in javadoc about the risk
| } | ||
| } | ||
|
|
||
| public void addResponseListener(final ResponseListener listener) { |
There was a problem hiding this comment.
consider nixing this as it is package private so people can't use it anyway.. makes the code smaller
There was a problem hiding this comment.
✅
This was a result of copy pasting from our repo where that was useful. Removed here since it isn't needed.
| trace = previousTrace != null ? previousTrace : trace; // Handles race condition | ||
|
|
||
| trace.spans.add(new DDMappingSpan(span)); | ||
| if (span.kind() == Span.Kind.SERVER |
| public static final long FLUSH_DELAY = TimeUnit.SECONDS.toMillis(1); | ||
|
|
||
| private final Queue<List<DDMappingSpan>> reportingTraces; | ||
| private final Map<String, PendingTrace> pendingTraces = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
this can potentially OOM on backlog. consider what to do about that (either don't use CHM or use some other sort of means to have an absolute limit)
There was a problem hiding this comment.
Growth is somewhat time constrained. I've documented this potential issue in the class comment.
| return System.nanoTime(); | ||
| } | ||
|
|
||
| private class PendingTrace { |
There was a problem hiding this comment.
static the things that don't need to be instance
There was a problem hiding this comment.
This is non-static because nanoTime is overridden in the tests to provide specific values.
| target = new MapPropertySource(PROPERTY_SOURCE_NAME, new HashMap<>()); | ||
| } | ||
|
|
||
| target.getSource().put("spring.sleuth.supports-join", false); |
There was a problem hiding this comment.
maybe you can also override the traceId128 thing I suppose. This won't always work, but anyway think about it..
There was a problem hiding this comment.
Not sure what you mean...
There was a problem hiding this comment.
does DD expect 128bit trace IDs? If so, there's a similar property for that
There was a problem hiding this comment.
DD only uses 64 bit trace/span id's.
| // from configuring a bean with the same name. | ||
| @Bean | ||
| public Reporter<Span> reporter() { | ||
| return new DatadogReporter(); |
There was a problem hiding this comment.
here's where builder would be helpful as some properties can be set in spring and mapped to builder methods here.
|
microseconds. theoretically it is possible to have nanos resolution clocks
but usually it isn't accurate to that resolution. i recall an issue in
htrace where they chose nanos to permit higher resolution clocks.
https://issues.apache.org/jira/browse/HTRACE-376
|
tylerbenson
force-pushed
the
initial-commit
branch
from
c7e2879 to
93db6cd
Compare
No description provided.