First cut at a Zipkin Span Exporter #1106

jkwatson · 2020-04-13T22:02:04Z

Resolves #534

Note: this code is heavily copied from the OpenCensus zipkin implementation.

jkwatson · 2020-04-13T22:08:59Z

@adriancole would love your eyes on this one!

build.gradle

codefromthecrypt

Thanks, I think I saw this code when in census, just a few clarifications

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

codefromthecrypt · 2020-04-14T00:35:10Z

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

+    }
+    Status status = spanData.getStatus();
+    if (status != null) {
+      spanBuilder.putTag(STATUS_CODE, status.getCanonicalCode().toString());


Is there a chance this can be an HTTP code? or is this RPC only?

the statuses are defined here:

opentelemetry-java/api/src/main/java/io/opentelemetry/trace/Status.java

Line 38 in c24e4a9

public final class Status {

this appears RPC status, can you confirm HTTP is not mapped to this?

Can confirm that HTTP is definitely mapped to these: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

.../zipkin/src/test/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfigurationTest.java

build.gradle

exporters/zipkin/build.gradle

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

codefromthecrypt

some README help

exporters/zipkin/README.md

carlosalberto · 2020-04-14T14:52:00Z

Not an expert of Zipkin, but overall looks great @jkwatson You need to add the new artifact to all/build.gradle too, btw ;)

jkwatson · 2020-04-14T16:43:33Z

@adriancole I think I got all of your questions/cleanups addressed but the one that I don't understand. Can you take a look?

codecov-io · 2020-04-14T16:45:59Z

Codecov Report

Merging #1106 into master will decrease coverage by 0.05%.
The diff coverage is 85.32%.

@@             Coverage Diff              @@
##             master    #1106      +/-   ##
============================================
- Coverage     85.56%   85.51%   -0.06%     
- Complexity     1097     1134      +37     
============================================
  Files           139      141       +2     
  Lines          4136     4245     +109     
  Branches        372      392      +20     
============================================
+ Hits           3539     3630      +91     
- Misses          454      463       +9     
- Partials        143      152       +9

Impacted Files	Coverage Δ	Complexity Δ
...telemetry/exporters/zipkin/ZipkinSpanExporter.java	`84.61% <84.61%> (ø)`	`35.00 <35.00> (?)`
.../exporters/zipkin/ZipkinExporterConfiguration.java	`100.00% <100.00%> (ø)`	`2.00 <2.00> (?)`
...elemetry/sdk/trace/export/BatchSpansProcessor.java	`88.48% <0.00%> (-1.05%)`	`10.00% <0.00%> (ø%)`

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 6e7f34d...813fb95. Read the comment docs.

codefromthecrypt

Most important feedback is to not leak tentative tag keys unless they are already in use. The suggestion is to use only "grpc.status_code" and "error" for now.

https://github.com/openzipkin/brave/blob/master/instrumentation/grpc/src/main/java/brave/grpc/GrpcParser.java#L66

This gives time to understand the nature of this, how widely used these status codes are etc, prior to adding load to people's indexes. Someone who wants to add more tags meanwhile can do that by wrapping this exporter in another one that adds a bunch of other tags.

exporters/zipkin/build.gradle

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

codefromthecrypt · 2020-04-14T23:52:40Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+  ZipkinExporterConfiguration() {}
+
+  /**
+   * Returns the service name.


I'm guessing the project has rules that require filler javadoc statements? These don't seem to add value otherwise, except for maybe the version once it is correct. If you want better descriptions let me know.

This was directly copied from the opencensus exporter. Better descriptions would be great!

And, I agree with you on the usefulness of much of the javadoc.

in that case, is it within the rules of the project to remove them? Less fluff to write, and we can focus on single-sentence explanations. lemme know and then I'll respond accordingly.

I'll poke at this tomorrow. I'm not 100% sure of the rules, myself.

Looks like javadoc is not required, but if it's there, it needs to be complete and well-formed.

ok I'll give you some text in another review round

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

exporters/zipkin/src/test/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporterTest.java

codefromthecrypt · 2020-04-17T01:42:54Z

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

+  // The naming follows Zipkin convention. As an example see:
+  // https://github.com/openzipkin/brave/blob/eee993f998ae57b08644cc357a6d478827428710/instrumentation/http/src/main/java/brave/http/HttpTags.java
+  // Note: these 3 fields are non-private for testing
+  static final String STATUS_CODE = "grpc.status_code";


@jkwatson PS I was assuming grpc "only" sets status vs adding a different tag also like "rpc.status" or "grpc.status" attribute per https://github.com/open-telemetry/opentelemetry-specification/blob/bfb060b23113ba9af492f8c63dd89ecfc500810b/specification/trace/semantic_conventions/rpc.md#status

In this case, I think the logic below can be simplified.. only when the mandatory "rpc.service" attribute exists, set "grpc.status_code" as we aren't likely to use the status values for any other reason.

The follow-up comment can refer to openzipkin/brave#999 which is likely to have the outcome of setting a tag like "rpc.status" (not yet decided) when the status is not OK. That is assuming there's no heuristic way to identify if the service in otel is grpc or not.

cool. I think I've got it in the right shape now!

codefromthecrypt

here ya go.

Glad I looked at this again (and you have patience for it) Noticed 2 glitches

v2Url is non conventional and misleading.. replace with "endpoint"
SpanBytesEncoder is unnecessarily narrow. Replace with BytesEncoder<Span>

codefromthecrypt · 2020-04-18T00:12:09Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+   * @return the service name.
+   * @since 0.4.0
+   */
+  public abstract String getServiceName();


delete need to expose this as it is conditional anyway by marking package private (delete javadoc)

codefromthecrypt · 2020-04-18T00:14:55Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+   * @return the Zipkin V2 URL.
+   * @since 0.4.0
+   */
+  public abstract String getV2Url();


delete need to expose this as it is conditional anyway by marking package private (delete javadoc)

codefromthecrypt · 2020-04-18T00:15:40Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+   * @return the Zipkin sender.
+   * @since 0.4.0
+   */
+  @Nullable


is this actually nullable? or would this just be UrlSender by default?

you can delete the javadoc by making this package private..

agreed. This isn't actually nullable.

BUT, autovalue needs it to be marked as such so that the builder will let me check to see if it's been set. 🤦

codefromthecrypt · 2020-04-18T00:17:30Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+     * @return this Builder instance.
+     * @since 0.4.0
+     */
+    public abstract Builder setServiceName(String serviceName);


From brave. (PS you will very likely end up needing localIP override. implicit lookups are a nice default, but users set this to the well-known-address sometimes.

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/Tracing.java#L163

/** * Label of the remote node in the service graph, such as "favstar". Avoid names with variables * or unique identifiers embedded. Defaults to "unknown". * * <p>This is a primary label for trace lookup and aggregation, so it should be intuitive and * consistent. Many use a name from service discovery. */

codefromthecrypt · 2020-04-18T00:19:47Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+ */
+@AutoValue
+@Immutable
+public abstract class ZipkinExporterConfiguration {


consider adding a static factory method to simplify this.

ex

/** * Builds a HTTP exporter for <a href="https://zipkin.io/zipkin-api/#/">Zipkin V2</a> format. * * @param endpoint See {@link Builder#setEndpoint(String)}, ex. "http://zipkinhost:9411/api/v2/spans". */ public static ZipkinExporterConfiguration create(String endpoint) { ... newbuilder.endpoint().build()

In fact.. if you do this, you can probably get out of the complex statements in the builder. remove the "endpoint" variant, and require anyone using the builder to supply their own sender and encoder. SOOO much simpler.

would you have an overload for the serviceName, or just require people who want to set that to use the builder directly?

I tried this out, including the serviceName option. Let me know what you think.

I guess the local service name is still not a otel concept right?

I think(?) this is the service.name resource attribute: https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions#service

If this is the same thing as the Resource service.name, then that is a property that is available to the exporter via the Resource attached to the SpanData instance. It's not generally something you configure at the exporter level, at least not from what I've seen so far.

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

codefromthecrypt · 2020-04-18T00:29:08Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+    public abstract Builder setV2Url(String v2Url);
+
+    /**
+     * Sets the Zipkin sender.


Implements the client side of the span transport. Defaults to {@link UrlConnectionSender}.

codefromthecrypt · 2020-04-18T00:31:12Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+     * @return this Builder instance
+     * @since 0.4.0
+     */
+    public abstract Builder setEncoder(SpanBytesEncoder encoder);


change to BytesEncoder<Span> as otherwise you'll break non-zipkin formats like https://github.com/openzipkin/zipkin-gcp/blob/master/sender-stackdriver/src/main/java/zipkin2/reporter/stackdriver/StackdriverEncoder.java

(ack that they would make their own exporter, but anyway no need to narrow the type)

codefromthecrypt · 2020-04-18T00:31:29Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+     * Sets the {@link SpanBytesEncoder}.
+     *
+     * @param encoder the {@code SpanBytesEncoder}.
+     * @return this Builder instance


Suggested change

* @return this Builder instance

* @return this Builder instance

* @see SpanBytesEncoder

codefromthecrypt · 2020-04-18T00:32:32Z

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

+    public abstract Builder setSender(Sender sender);
+
+    /**
+     * Sets the {@link SpanBytesEncoder}.


Controls the format used by the {@link Sender}. Defaults to {@link SpanBytesEncoder#JSON_V2}

codefromthecrypt · 2020-04-21T01:06:02Z

sorry John. I suspect hat otel doesnt currently have a standard tag or property for the service name so it would only end up valid if also specified here. this is a but of a dilemma but can be punted. fwiw I have noticed many vendors and certainly jaeger have a local service name property. if this could be back filled into the core api then the factory here would be far simpler as it would just note this inherits the service name property into the local endpoint by default. this is far better than "unknown" eventhough a builder override should still be possible for that.

assuming there isnt a standard property the way i would handle it is newBuilder(sender) and newBuilder(endpoint) overloads vs a create method that has two strings. once the model can have a standard way to get the local service name we can add the simplified factory method then.

wdyt?

jkwatson · 2020-04-21T02:28:00Z

sorry John. I suspect hat otel doesnt currently have a standard tag or property for the service name so it would only end up valid if also specified here. this is a but of a dilemma but can be punted. fwiw I have noticed many vendors and certainly jaeger have a local service name property. if this could be back filled into the core api then the factory here would be far simpler as it would just note this inherits the service name property into the local endpoint by default. this is far better than "unknown" eventhough a builder override should still be possible for that.

assuming there isnt a standard property the way i would handle it is newBuilder(sender) and newBuilder(endpoint) overloads vs a create method that has two strings. once the model can have a standard way to get the local service name we can add the simplified factory method then.

wdyt?

So, as mentioned above, the service.name is not a global thing in OTel, but provided via the Resource that is attached to each Span for export. That doesn't really jive with the code to look up the local endpoint, since that's more exporter-scoped, rather than span scoped.

Thoughts on dropping the local endpoint code, and just attaching the service name provided via the Resource object?

codefromthecrypt · 2020-04-21T02:41:55Z

Thoughts on dropping the local endpoint code, and just attaching the service name provided via the Resource object?

We use a model instead of tags for this, if we don't map anything, not just search but aggregations won't work. IOTW "service.name" as far as I can understand would map to localEndpoint.serviceName (and should not also be added as a tag as that wastes overhead).

If that's in, it sounds fine to take out builder-related code for setting a default endpoint. In brave, for example, we just use builder stuff as a default anyway. You can expect a request at some point to override this due to well-known historical problems with making service span-scoped as opposed to local-root scoped *. However, sounds fine to me to do that later provided
localEndpoint.serviceName is set (and you are sure that "service.name" is routinely defaulted in tracer setup)

servicename was set at tracer scope default to help users out of very big holes dug by setting service names randomly and sporadically throughout a local root. IOTW we used to do the tag based approach and it resulted in chaos

codefromthecrypt · 2020-04-21T02:50:11Z

ps I suppose another way vs a setting here could be that just a decorating span exporter. I suspect that the random service name problem, which wreaks havoc on dependency trees, search indexes etc, will be not just limited to Zipkin.

If there's no way to default the tracer (maybe there is) an example (or institutional knowledge) of a wrapping SpanExporter that overrides the service tag would also work fine once folks see everything broken.

codefromthecrypt · 2020-04-21T02:54:10Z

TL;DR;

minimum: map "service.name" -> localEndpoint.serviceName and drop the tag

the inconsistent "service.name" problem is out of scope for the exporter anyway, and should be watched carefully, especially if never set.

once minimum is in, it is time to move past the "eyeball code" phase into seeing it work. Basically whatever simple app you have that generates a trace across two services, invoke that and either paste a screenshot of lens, or the actual trace json sent from both the nodes. I can verify personally on behalf of the team, and you can

sg?

jkwatson · 2020-04-21T04:04:16Z

Once we get this merged, I can point a test service at New Relic's zipkin span ingest and see how it all looks.

jkwatson · 2020-04-21T17:46:15Z

ok. I got rid of the exporter-level Endpoint code, and just use the Resource to populate an Endpoint.serviceName . This is probably down the bare-bones implementation at this point, which is probably a good starting point. Additional features can definitely be requested & added in the future!

codefromthecrypt · 2020-04-22T00:34:09Z

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

+  }
+
+  static Span generateSpan(SpanData spanData) {
+    Endpoint endpoint = EMPTY_ENDPOINT;


please don't add an empty endpoint

The zipkin API just sets null when you pass in an empty one. Are you saying that if we can't make an endpoint, we shouldn't send spans?

it is distracting to set an empty endpoint for reasons including what you mentioned. that's the main point. just call the endpoint builder when you have one?

the longer answer is that it would be easy for this code to drift to adding more data into the span for no reason. For example, if you change codec later, it would likely serialize "localEndpoint": {} which is useless overhead, plus adding empty is already confusing.

hope this makes sense.

codefromthecrypt

we need to add a service name fallback

codefromthecrypt · 2020-04-22T00:36:24Z

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

+  static Span generateSpan(SpanData spanData) {
+    Endpoint endpoint = EMPTY_ENDPOINT;
+    Map<String, AttributeValue> resourceAttributes = spanData.getResource().getAttributes();
+    AttributeValue serviceNameValue = resourceAttributes.get("service.name");


use

opentelemetry-java/sdk/src/main/java/io/opentelemetry/sdk/resources/ResourceConstants.java

Line 32 in 6330901

public static final String SERVICE_NAME = "service.name";

?

codefromthecrypt · 2020-04-22T00:44:42Z

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

+
+  static Span generateSpan(SpanData spanData) {
+    Endpoint endpoint = EMPTY_ENDPOINT;
+    Map<String, AttributeValue> resourceAttributes = spanData.getResource().getAttributes();


I understand where this is coming from and the discussions around it, but I still haven't heard any reason to believe "service.name" would be set by default. I suspect that's why the jaeger export was written to have a gear for this.

opentelemetry-java/exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/JaegerIntegrationTest.java

Line 85 in c32c776

.setServiceName(SERVICE_NAME)

Recall the main thing we've done recently is dodge a 2 arg factory method. To make data unqueryable is a more bitter pill than just deleting that discussion and having no factory methods. Basically if this is all about the single arg factory method, it really isn't worth it to cause damage.

A different point.. the whole thing started about the choice between endpoint and a different sender. This is not a complex enough problem to make us want to damage the data model by removing something more important for. If it is, simply remove the endpoint arg to "buy us" a service name one.

If it isn't a problem to have three methods (one for service, one for endpoint shortcut and one for the sender that uses) do an overload? Ex we do similarly in our okhttp sender (also autovalue)

/** * No default. The POST URL for zipkin's <a href="https://zipkin.io/zipkin-api/#/">v2 api</a>, * usually "http://zipkinhost:9411/api/v2/spans" */ // customizable so that users can re-map /api/v2/spans ex for browser-originated traces public Builder endpoint(String endpoint) { if (endpoint == null) throw new NullPointerException("endpoint == null"); HttpUrl parsed = HttpUrl.parse(endpoint); if (parsed == null) throw new IllegalArgumentException("invalid post url: " + endpoint); return endpoint(parsed); }

The reason I'm so concerned basically is I cannot see anything in this repo that sets this and we know lack thereof results in unqueryable and unaggregatable data. Using a dummy name which cannot be set at a level people are likely to control will result in an unfairly bad experience vs jaeger who have a setting. This is not worth waiting for complaints about (which likely would dump into zipkin's channel)

Regardless, if we want to suggest to prioritize the "service.name", knowing we have a fallback, both should be in the README. When there's a normalized way to set the "service.name" in the span resource above this tier, both jaeger and zipkin exporters can get a README update.

my first stab at writing that comment was crap.. hopefully my rewording above is more coherent (coffee is setting in)

The idea, as I understand it, is that the person configuring the SDK will add the Resource to the TracerProvider once, at startup. It's not code that would exist in the SDK itself, but written by the SDK user.

I'm not sure about that jaeger exporter, and why it might have been written that way. My guess is that it predates the semantic convention of putting service.name in the Resource.

I'll put an exporter-level serviceName back and have it be the fallback.

There is an issue to use the resource for that in Jaeger

cool maybe link to that in a TODO. regardless it seems a fallback is prudent.

There are some concerns that you also pointed. We should resolve both exporters when we do it.

Also we have this defined in specs as well:
open-telemetry/opentelemetry-specification#472

@adriancole may want to confirm that it is what you expect :). Thanks

ok, I restored the serviceName option, but have it be superceded by the service.name set in the Resource, if it's there. Phew.

If this is an ok approach for now, then let's get this bad boy merged and see if it works!

…ean up other unit tests

…in/ZipkinExporterConfiguration.java Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com>

exporters/zipkin/README.md

codefromthecrypt

fan of most things, except the factory methods, but that you continue putting them back suggests you feel otherwise :)

I'll leave it up to you vs tie you up on another round.

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java

jkwatson · 2020-04-23T15:39:24Z

ok! I think I've got it in a good state. Looking forward to kicking the tires and see how it works.

codefromthecrypt

update top-level README, QUICKSTART and CHANGELOG files?

Also, the jaeger example seems extremely copy/paste find replace, why not do that with a default endpoint of http://localhost:9411/api/v2/spans?

I think in the jaeger exporter README they link to this example, which would allow others to see if it works or not for lack of something more integrated.

https://github.com/open-telemetry/opentelemetry-java/blob/8a0ec1d42dbb39e24507b75c629ce44f6dfd9ba5/examples/jaeger/src/main/java/io/opentelemetry/example/JaegerExample.java

jkwatson · 2020-04-24T19:25:41Z

I created an issue for the zipkin example: #1137

jkwatson · 2020-04-24T19:26:20Z

ok, I think this is ready to go. @approvers Can you give it a once over, so we can make this a part of the 0.4.0 release next week?

jkwatson · 2020-04-27T16:12:23Z

@bogdandrutu @carlosalberto review, please, so we can get this merged for 0.4.0.

codefromthecrypt

thanks for this @jkwatson!

* basic zipkin span exporter with some unit tests * add tests for the configuration class, hook up a create method and clean up other unit tests * add a README * Update exporters/zipkin/README.md Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com> * Update exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com> * Update exporters/zipkin/README.md Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com> * apply PR review comments * Update build.gradle Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com> * a little bit of cleanup from PR review * implement shutdown and provide javadoc about the closing of the Sender * don't set attributes if they're already set. * formatting * grpc tweaks, and an exception thrown * Update the comment to be a little more accurate. * formatting * Update exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com> * Update exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com> * doc/naming cleanup * simplify the builder, provide two simple factory methods * strip out the endpoint logic and instead get it from the Resource * formatting * restore the serviceName option, but keep the Resource-based override. * update for changes from master. * remove factory methods on the configuration, and update the README * update the docs to match the requirements * fix a typo * tiny re-arrange of javadoc * Add zipkin to the top-level docs Co-authored-by: Adrian Cole <adriancole@users.noreply.github.com>

jkwatson requested review from arminru, bogdandrutu, carlosalberto, pavolloffay, songy23 and tylerbenson as code owners April 13, 2020 22:02

bogdandrutu reviewed Apr 13, 2020

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

codefromthecrypt reviewed Apr 14, 2020

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

codefromthecrypt reviewed Apr 14, 2020

View reviewed changes

exporters/zipkin/README.md Outdated Show resolved Hide resolved

exporters/zipkin/README.md Outdated Show resolved Hide resolved

codefromthecrypt reviewed Apr 15, 2020

View reviewed changes

codefromthecrypt reviewed Apr 17, 2020

View reviewed changes

codefromthecrypt reviewed Apr 18, 2020

View reviewed changes

fabienpomerol mentioned this pull request Apr 21, 2020

Add Zipkin Exporter Support fabienpomerol/vertx-opentelemetry#3

Open

codefromthecrypt reviewed Apr 22, 2020

View reviewed changes

jwatson added 3 commits April 22, 2020 14:39

basic zipkin span exporter with some unit tests

5a72b68

add tests for the configuration class, hook up a create method and cl…

3b3dc96

…ean up other unit tests

add a README

cb4cf58

jwatson and others added 11 commits April 22, 2020 14:39

grpc tweaks, and an exception thrown

4a45183

Update the comment to be a little more accurate.

ab0f7aa

formatting

16f0907

Update exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipk…

b5ede79

…in/ZipkinExporterConfiguration.java Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com>

Update exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipk…

86a4f46

…in/ZipkinExporterConfiguration.java Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com>

doc/naming cleanup

5e4416c

simplify the builder, provide two simple factory methods

24b7d2a

strip out the endpoint logic and instead get it from the Resource

0c04e5c

formatting

ca8d565

restore the serviceName option, but keep the Resource-based override.

b1a7c37

update for changes from master.

8112b7a

jkwatson force-pushed the zipkin_exporter branch from 528e204 to 8112b7a Compare April 22, 2020 21:57

jkwatson mentioned this pull request Apr 22, 2020

Contrib packages should not use "internal" API package #1128

Closed

codefromthecrypt reviewed Apr 22, 2020

View reviewed changes

exporters/zipkin/README.md Outdated Show resolved Hide resolved

codefromthecrypt approved these changes Apr 22, 2020

View reviewed changes

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java Outdated Show resolved Hide resolved

...ters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinExporterConfiguration.java Outdated Show resolved Hide resolved

jwatson added 3 commits April 23, 2020 08:27

remove factory methods on the configuration, and update the README

7ca4bf9

update the docs to match the requirements

5fee364

fix a typo

c660d99

tiny re-arrange of javadoc

813fb95

codefromthecrypt approved these changes Apr 23, 2020

View reviewed changes

Add zipkin to the top-level docs

574542e

codefromthecrypt approved these changes Apr 28, 2020

View reviewed changes

bogdandrutu approved these changes May 1, 2020

View reviewed changes

bogdandrutu merged commit 0ce1a38 into open-telemetry:master May 1, 2020

jkwatson added this to the May Release milestone May 1, 2020

	* @return this Builder instance
	* @return this Builder instance
	* @see SpanBytesEncoder

First cut at a Zipkin Span Exporter #1106

First cut at a Zipkin Span Exporter #1106

Conversation

jkwatson commented Apr 13, 2020 • edited Loading

jkwatson commented Apr 13, 2020

codefromthecrypt 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

codefromthecrypt left a comment

Choose a reason for hiding this comment

carlosalberto commented Apr 14, 2020

jkwatson commented Apr 14, 2020

codecov-io commented Apr 14, 2020 • edited Loading

Codecov Report

codefromthecrypt 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

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

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt Apr 18, 2020 • edited Loading

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

Choose a reason for hiding this comment

codefromthecrypt Apr 18, 2020 • edited Loading

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

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

codefromthecrypt commented Apr 21, 2020

jkwatson commented Apr 21, 2020

codefromthecrypt commented Apr 21, 2020

codefromthecrypt commented Apr 21, 2020

codefromthecrypt commented Apr 21, 2020

jkwatson commented Apr 21, 2020

jkwatson commented Apr 21, 2020

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

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt Apr 22, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

jkwatson commented Apr 23, 2020

codefromthecrypt left a comment

Choose a reason for hiding this comment

jkwatson commented Apr 24, 2020

jkwatson commented Apr 13, 2020 •

edited

Loading

codecov-io commented Apr 14, 2020 •

edited

Loading

codefromthecrypt Apr 18, 2020 •

edited

Loading

codefromthecrypt Apr 18, 2020 •

edited

Loading

codefromthecrypt Apr 22, 2020 •

edited

Loading