Add brave module which supersedes zipkin module #1840

minwoox · 2019-06-14T03:42:12Z

Motivation:
As @anuraaga comments, it's not Zipkin, but Brave we're interfacing.

Modifications:

Move internal.tracing package in zipkin module to internal.brave in brave module.
Deprecation
- armeria-brave supersedes armeria-zipkin.
- BraveClient and BraveService supersedes TracingHttpClient and TracingHttpService
- RequestContextCurrentTraceContext in brave supersedes the same class in zipkin
Fix a bug where decorateScope is not called.
Miscellaneous
- Add missing equals in DefaultAggregatedHttpRequest
- Update Brave from 5.6.5 -> 5.6.6

Result:

armeria-brave supersedes armeria-zipkin.
A user can now decorate a scope when it's created.
Close Add HttpTracingClient/ServiceBuilder #1792 anyway.
Brave 5.6.5 -> 5.6.6

zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java

codecov · 2019-06-14T04:19:53Z

Codecov Report

Merging #1840 into master will increase coverage by 0.04%.
The diff coverage is 77.25%.

@@             Coverage Diff              @@
##             master    #1840      +/-   ##
============================================
+ Coverage     73.07%   73.12%   +0.04%     
- Complexity     8838     8880      +42     
============================================
  Files           784      788       +4     
  Lines         34698    34876     +178     
  Branches       4239     4262      +23     
============================================
+ Hits          25356    25503     +147     
- Misses         7186     7213      +27     
- Partials       2156     2160       +4

Impacted Files	Coverage Δ	Complexity Δ
.../armeria/internal/brave/AsciiStringKeyFactory.java	`100% <ø> (ø)`	`2 <0> (?)`
.../com/linecorp/armeria/internal/brave/SpanTags.java	`80.43% <ø> (ø)`	`9 <0> (?)`
...orp/armeria/common/sse/DefaultServerSentEvent.java	`41.93% <0%> (ø)`	`7 <0> (ø)`	⬇️
...p/armeria/common/DefaultAggregatedHttpRequest.java	`30.76% <0%> (-69.24%)`	`6 <0> (ø)`
...com/linecorp/armeria/common/DefaultRpcRequest.java	`73.17% <0%> (ø)`	`13 <0> (ø)`	⬇️
...corp/armeria/client/tracing/HttpTracingClient.java	`92.15% <100%> (+0.32%)`	`13 <2> (ø)`	⬇️
...orp/armeria/server/tracing/HttpTracingService.java	`100% <100%> (ø)`	`10 <0> (ø)`	⬇️
...necorp/armeria/internal/brave/SpanContextUtil.java	`100% <100%> (ø)`	`3 <1> (?)`
...ecorp/armeria/internal/brave/TraceContextUtil.java	`100% <100%> (ø)`	`3 <3> (?)`
...om/linecorp/armeria/server/brave/BraveService.java	`100% <100%> (ø)`	`10 <10> (?)`
... and 12 more

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 294e987...4c98058. Read the comment docs.

zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java

zipkin/src/main/java/com/linecorp/armeria/common/tracing/AbstractTracingBuilder.java

zipkin/src/main/java/com/linecorp/armeria/server/tracing/TracingService.java

zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java

trustin

Nice work! 🚀

zipkin/src/main/java/com/linecorp/armeria/common/tracing/AbstractTracingBuilder.java

anuraaga

My initial scan of this made me think its fine and I delayed taking a look but I'm having second thoughts now, sorry :(

I think it's very rare for an application to have multiple Tracing instances - if they are all not configured exactly the same, there will be problems. As there will almost always be at least a server and a client, I think we're encouraging code duplication by having methods that don't accept Tracing as both decorators will have to have the same method calls, and as Tracing has some state like pending spans, it'll probably be less efficient to have them split up too. If this was an advanced case it's not so bad but it's the common case so I don't know how good it is to have these builders.

@adriancole Is it sane in the first place to have multiple Tracing instances in an app?

core/src/main/java/com/linecorp/armeria/common/DefaultAggregatedHttpRequest.java

zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java

minwoox · 2019-06-18T05:20:43Z

@anuraaga I agree with you on that. Probably it's better not to have these builders because there's no way to share the same Tracing.

minwoox · 2019-06-18T05:37:41Z

So let me remove the builders and leave the bug fix and renaming things 😉 Thanks!

codefromthecrypt · 2019-06-18T06:07:13Z

I agree: multiple tracing instances is usually a problem and we wouldnt want to guide folks towards this as it unleashes all sorts of questions such as whose threadlocal is this anyway? :)

…

On Tue, Jun 18, 2019, 12:13 PM Anuraag Agrawal ***@***.***> wrote: ***@***.**** commented on this pull request. My initial scan of this made me think its fine and I delayed taking a look but I'm having second thoughts now, sorry :( I think it's very rare for an application to have multiple Tracing instances - if they are all not configured exactly the same, there will be problems. As there will almost always be at least a server and a client, I think we're encouraging code duplication by having methods that don't accept Tracing as both decorators will have to have the same method calls, and as Tracing has some state like pending spans, it'll probably be less efficient to have them split up too. If this was an advanced case it's not so bad but it's the common case so I don't know how good it is to have these builders. @adriancole <https://github.com/adriancole> Is it sane in the first place to have multiple Tracing instances in an app? ------------------------------ In core/src/main/java/com/linecorp/armeria/common/DefaultAggregatedHttpRequest.java <#1840 (comment)>: > + + @OverRide + public int hashCode() { + int result = headers().hashCode(); + result = 31 * result + content().hashCode(); + result = 31 * result + trailers().hashCode(); + return result; + } + + @OverRide + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + + if (!(obj instanceof DefaultAggregatedHttpRequest)) { Should equality compare against HttpRequest instead of the implementation? What if there's another implementation that should compare equal? ------------------------------ In zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java <#1840 (comment)>: > */ -public class HttpTracingClient extends SimpleDecoratingClient<HttpRequest, HttpResponse> { ***@***.*** +public final class HttpTracingClient extends SimpleDecoratingClient<HttpRequest, HttpResponse> { What do you think about extending TracingClient until this class is removed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1840?email_source=notifications&email_token=AAAPVVZTF6KVXIQ7SGFJLBLP3BOFRA5CNFSM4HYD3LPKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3Z2I7A#pullrequestreview-250848380>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV3XJCYFH5YN73TCEHTP3BOFRANCNFSM4HYD3LPA> .

ikhoon

Good! 👍

ikhoon · 2019-06-19T05:26:34Z

@minwoox but codecov/patch shows the coverage of diff(63.2%) is lower than the target one.

codefromthecrypt · 2019-06-19T05:54:23Z

zipkin/src/main/java/com/linecorp/armeria/client/tracing/TracingClient.java

+    /**
+     * Creates a new tracing {@link Client} decorator using the specified {@link Tracing} instance.
+     */
+    public static Function<Client<HttpRequest, HttpResponse>, TracingClient> newDecorator(Tracing tracing) {


note we still have an issue to accept HttpTracing.. even if we aren't implementing anything with that, it might make sense to use this as an arg to the decorator (internally calling httpTracing.tracing()) rather than go back and overload/deprecate later

codefromthecrypt · 2019-06-19T05:54:50Z

zipkin/src/main/java/com/linecorp/armeria/client/tracing/TracingClient.java

+     * and the remote service name.
+     */
+    public static Function<Client<HttpRequest, HttpResponse>, TracingClient> newDecorator(
+            Tracing tracing, @Nullable String remoteServiceName) {


if this had HttpTracing, the removeServiceName arg would be redundant and unnecessary as that's a part of HttpTracing

codefromthecrypt

made a note for consideration of future-proofing things. since this is api break anyway we can take advantage of this to move to HttpTracing as the graph supplied to the factory (instead of the Tracing graph).

This will give us most flexibility later

trustin · 2019-06-24T03:33:10Z

How about moving to tracing.zipkin (or just to zipkin) and renaming to ZipkinClient/Service by taking advantage of this breaking change, then? We may need to add tracing.opentracing or other vendor-specific implementations later.

minwoox · 2019-06-28T07:21:53Z

@minwoox but codecov/patch shows the coverage of diff(63.2%) is lower than the target one.

@ikhoon Increased the coverage. 😉

brave/src/main/java/com/linecorp/armeria/client/brave/BraveClient.java

brave/src/main/java/com/linecorp/armeria/common/brave/RequestContextCurrentTraceContext.java

brave/src/main/java/com/linecorp/armeria/server/brave/BraveService.java

brave/src/main/java/com/linecorp/armeria/client/brave/BraveClient.java

Motivation: A user currently has to build `Tracing` by him/herself and set `currentTraceContext` manually. We could add `HttpTracing(Client|Service)Builder` which builds a `Tracing` for a user. Modifications: - Add `HttpTracing(Client|Service)`Builder - May fix a bug where `decorateScope` is not called - Deprecation - The consturctor of `HttpTracingService` is not public anymore. A user should use the builder or the static factory method. Result: - A user can easidy build an `HttpTracing(Client|Service)` using builders.

minwoox · 2019-07-01T09:22:12Z

@anuraaga Fixed. PTAL 🙇

brave/build.gradle

brave/src/main/java/com/linecorp/armeria/client/brave/package-info.java

zipkin/build.gradle

zipkin/src/main/java/com/linecorp/armeria/common/tracing/RequestContextCurrentTraceContext.java

zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java

minwoox · 2019-07-01T09:54:18Z

@trustin All fixed. PTAL 🙇

trustin

👍

ikhoon

Thanks! @minwoox 👍

minwoox · 2019-07-02T06:53:41Z

Thanks for reviewing. 😉

@anuraaga

Motivation: As @anuraaga comments, it's not Zipkin, but Brave we're interfacing. Modifications: - Move `internal.tracing` package in `zipkin` module to `internal.brave` in `brave` module. - Deprecation - `armeria-brave` supersedes `armeria-zipkin`. - `BraveClient` and `BraveService` supersedes `TracingHttpClient` and `TracingHttpService` - `RequestContextCurrentTraceContext` in `brave` supersedes the same class in `zipkin` - Fix a bug where `decorateScope` is not called. - Miscellaneous - Add missing `equals` in `DefaultAggregatedHttpRequest` - Update Brave from 5.6.5 -> 5.6.6 Result: - `armeria-brave` supersedes `armeria-zipkin`. - A user can now decorate a scope when it's created. - Close line#1792 anyway. - Brave 5.6.5 -> 5.6.6

minwoox requested review from hyangtack, ikhoon and trustin as code owners June 14, 2019 03:42

minwoox commented Jun 14, 2019

View reviewed changes

zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java Outdated Show resolved Hide resolved

minwoox requested a review from anuraaga June 14, 2019 03:44

minwoox added the new feature label Jun 14, 2019

minwoox added this to the 0.88.0 milestone Jun 14, 2019

minwoox force-pushed the add_tracing_builder branch from abf601c to f75b932 Compare June 14, 2019 03:48

minwoox force-pushed the add_tracing_builder branch from f75b932 to 9619add Compare June 14, 2019 07:32

minwoox changed the title ~~Add HttpTracing(Client|Service)Builder~~ Add HttpTracingClientBuilder and TracingServiceBuilder Jun 14, 2019

minwoox force-pushed the add_tracing_builder branch from 9619add to aea8255 Compare June 14, 2019 07:36

trustin requested changes Jun 17, 2019

View reviewed changes

minwoox changed the title ~~Add HttpTracingClientBuilder and TracingServiceBuilder~~ Add TracingClientBuilder and TracingServiceBuilder Jun 17, 2019

minwoox force-pushed the add_tracing_builder branch from 52f8559 to dce924f Compare June 17, 2019 11:37

trustin approved these changes Jun 18, 2019

View reviewed changes

zipkin/src/main/java/com/linecorp/armeria/common/tracing/AbstractTracingBuilder.java Outdated Show resolved Hide resolved

anuraaga reviewed Jun 18, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/DefaultAggregatedHttpRequest.java Outdated Show resolved Hide resolved

zipkin/src/main/java/com/linecorp/armeria/client/tracing/HttpTracingClient.java Show resolved Hide resolved

minwoox force-pushed the add_tracing_builder branch 2 times, most recently from bd72fad to 7fd864a Compare June 18, 2019 10:09

minwoox changed the title ~~Add TracingClientBuilder and TracingServiceBuilder~~ Fix a bug where ScopeDecorator.decorateScope is not invoked Jun 18, 2019

ikhoon approved these changes Jun 19, 2019

View reviewed changes

anuraaga approved these changes Jun 19, 2019

View reviewed changes

codefromthecrypt reviewed Jun 19, 2019

View reviewed changes

minwoox changed the title ~~Fix a bug where ScopeDecorator.decorateScope is not invoked~~ Add brave module which supersedes zipkin module Jun 28, 2019

anuraaga reviewed Jun 28, 2019

View reviewed changes

trustin requested review from anuraaga, ikhoon and trustin June 28, 2019 08:45

minwoox added 2 commits July 1, 2019 17:53

Add brave module

5a79d60

minwoox force-pushed the add_tracing_builder branch 3 times, most recently from 5dea9f2 to 74f4c3e Compare July 1, 2019 09:13

Address comments by @anuraaga

f9ee5d1

minwoox force-pushed the add_tracing_builder branch from 74f4c3e to f9ee5d1 Compare July 1, 2019 09:14

trustin requested changes Jul 1, 2019

View reviewed changes

Address comments by @trustin

67ee121

trustin approved these changes Jul 2, 2019

View reviewed changes

ikhoon approved these changes Jul 2, 2019

View reviewed changes

minwoox force-pushed the add_tracing_builder branch from 578dee9 to d56241e Compare July 2, 2019 06:07

Update version

4c98058

minwoox force-pushed the add_tracing_builder branch from d56241e to 4c98058 Compare July 2, 2019 06:14

anuraaga approved these changes Jul 2, 2019

View reviewed changes

trustin added defect dependencies labels Jul 2, 2019

trustin merged commit fd8b57e into line:master Jul 2, 2019

minwoox deleted the add_tracing_builder branch July 2, 2019 06:53

trustin added the deprecation label Jul 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add brave module which supersedes zipkin module #1840

Add brave module which supersedes zipkin module #1840

minwoox commented Jun 14, 2019 •

edited by trustin

codecov bot commented Jun 14, 2019 •

edited

trustin left a comment

anuraaga left a comment

minwoox commented Jun 18, 2019

minwoox commented Jun 18, 2019

codefromthecrypt commented Jun 18, 2019 via email

ikhoon left a comment

ikhoon commented Jun 19, 2019 •

edited

codefromthecrypt Jun 19, 2019

codefromthecrypt Jun 19, 2019

codefromthecrypt left a comment

trustin commented Jun 24, 2019

minwoox commented Jun 28, 2019

minwoox commented Jul 1, 2019

minwoox commented Jul 1, 2019

trustin left a comment

ikhoon left a comment

minwoox commented Jul 2, 2019

Add brave module which supersedes zipkin module #1840

Add brave module which supersedes zipkin module #1840

Conversation

minwoox commented Jun 14, 2019 • edited by trustin

codecov bot commented Jun 14, 2019 • edited

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

minwoox commented Jun 18, 2019

minwoox commented Jun 18, 2019

codefromthecrypt commented Jun 18, 2019 via email

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon commented Jun 19, 2019 • edited

codefromthecrypt Jun 19, 2019

Choose a reason for hiding this comment

codefromthecrypt Jun 19, 2019

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

trustin commented Jun 24, 2019

minwoox commented Jun 28, 2019

minwoox commented Jul 1, 2019

minwoox commented Jul 1, 2019

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Jul 2, 2019

minwoox commented Jun 14, 2019 •

edited by trustin

codecov bot commented Jun 14, 2019 •

edited

ikhoon commented Jun 19, 2019 •

edited