Introduce `BraveRpcService` by jrhee17 · Pull Request #6115 · line/armeria

jrhee17 · 2025-02-20T03:02:35Z

Motivation:

The motivation for this PR is better described in #6084

The changeset in this PR attempts to:

Expose ArmeriaHttpServerParser and ArmeriaRpcServerParser
- By doing so, users can choose what information must be extracted from BraveService or BraveRpcService. This can provide more flexibility on whether to use only BraveService, only BraveRpcService, or both BraveService and BraveRpcService
Introduce BraveRpcService which allows users to perform sampling or request/response parsing based on RpcRequest

Modifications:

Added BraveRpcService. By default, armeria-specific tags/annotations recorded by BraveRpcService is the same as BraveService
Exposed ArmeriaHttpServerParser and ArmeriaRpcServerParser to allow users to easily construct a RpcTracing or HttpTracing

Result:

Users may use RpcRequest, RpcResponse to apply sampling/tags/annotations

github-actions · 2025-02-20T03:27:39Z

🔍 Build Scan® (commit: `23843d7`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/rlsxmw4wulbp4
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/jp6unhsqqjg7o
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/edgbxw24cdh42
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/knfyzmygjbqwq
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/y47gtdxzy5pae
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/2v2ceryqy2jei
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/esafiohaf5egy

ikhoon · 2025-02-24T04:21:45Z

+ * Decorates an {@link RpcService} to trace inbound {@link RpcRequest}s using
+ * <a href="https://github.com/openzipkin/brave">Brave</a>.
+ */
+public final class BraveRpcService extends SimpleDecoratingRpcService {


Question) Is it possible to share the common logic with BraveService by adding AbstractBraveService and extending it, as you did in AbstractMetricCollectingClient?

ikhoon · 2025-03-06T02:41:21Z

+        super(delegate);
+    }
+
+    O serve0(ServiceRequestContext ctx, I req, BI braveReq, Span span) throws Exception {


This abstraction is slightly different from what I had in mind.
Instead of enforcing the subclass to call serve0() after preprocessing, should we handle all processes in AbstractBraveService but delegate the domain-specific part to the subclasses?

What do you think of this direction?

abstract class AbstractBraveService<BI extends brave.Request, BO extends brave.Response, ...> abstract BI braveRequest(ServiceRequestContext ctx, I req); abstract BO braveResponse(ServiceRequestContext ctx, RequestLog log, BI braveReq); abstract Span handleReceive(BI braveReq); abstract void handleSend(BO response, Span span); @Override public final O serve(ServiceRequestContext ctx, I req) throws Exception { if (!ctx.config().transientServiceOptions().contains(TransientServiceOption.WITH_TRACING)) { return unwrap().serve(ctx, req); } final BI braveReq = braveRequest(ctx, req); final Span span = handleReceive(braveReq); ... }

I see, I understood the intention was to open the possibility to support more request types. Updated

Previously, IIRC, there was some duplicate code and there were functions for which subclasses had to call parent methods when implementing them.
My proposal is to allow implementations to focus only on domain-specific parts, without such constraints.

ikhoon

Thanks, @jrhee17! 🙇‍♂️🙇‍♂️

minwoox

Left some questions, but basically looks good. 👍

minwoox · 2025-04-08T01:11:07Z

+    abstract Tracer tracer();
+
+    abstract RequestContextCurrentTraceContext currentTraceContext();
+


Tracer and currentTraceContext are only used in AbstractBraverService, so how about passing them through the constructor instead?

minwoox · 2025-04-08T01:19:20Z

 * </ul>
 */
-final class ArmeriaHttpServerParser implements HttpRequestParser, HttpResponseParser {
+public final class ArmeriaHttpServerParser {


Global comment: Could you add @UnstableApi?

minwoox · 2025-04-08T01:29:35Z

+        @Nullable
+        @Override
+        public String errorCode() {
+            return null;


Question: Is returning null okay?

I'm not sure if there is a way to reliably fetch an error code for thrift (or is there even a concept of error codes in thrift?)

I wasn't aware of that either. Let's fix this later if it becomes an issue.

minwoox · 2025-04-08T02:07:29Z

+        @Nullable
+        @Override
+        public Throwable error() {
+            final Object content = log.responseContent();


Can't we use log.responseCause?

trustin · 2025-04-08T02:12:43Z

+ * Copyright 2025 LINE Corporation
+ *
+ * LINE Corporation licenses this file to you under the Apache License,


Time to update the template?

Suggested change

* Copyright 2025 LINE Corporation

*

* LINE Corporation licenses this file to you under the Apache License,

* Copyright 2025 LY Corporation

*

* LY Corporation licenses this file to you under the Apache License,

trustin · 2025-04-08T02:13:40Z

+    abstract BI braveRequest(ServiceRequestContext ctx);
+
+    abstract BO braveResponse(ServiceRequestContext ctx, RequestLog log, BI braveReq);
+
+    abstract Span handleReceive(BI braveReq);
+
+    abstract void handleSend(BO response, Span span);


Shall we move these abstract methods down so they are grouped?

trustin · 2025-04-08T02:17:30Z

 * </ul>
 */
-final class ArmeriaHttpServerParser implements HttpRequestParser, HttpResponseParser {
+public final class ArmeriaHttpServerParser {


Maybe just BraveServerParsers? Any better names? 🤔

trustin · 2025-04-08T02:18:57Z

+ *   <li>address.local</li>
+ * </ul>
+ */
+public final class ArmeriaRpcServerParser {


@UnstableApi

How about merging this class into ArmeriaHttpServerParser and provide all factory methods there? e.g. BraveServerParsers.rpcRequestParser()

How about merging this class into ArmeriaHttpServerParser and provide all factory methods there?

I was assuming that most users will be interested in Http related integrations only.
Do you prefer that libs.brave6.instrumentation.rpc is included with the api scope instead?

How about merging this class into ArmeriaHttpServerParser and provide all factory methods there? e.g. BraveServerParsers.rpcRequestParser()

Just merged the class and added the dependency since it seems you prefer this way.

trustin · 2025-04-08T02:20:18Z

+ * Copyright 2019 LINE Corporation
+ *
+ * LINE Corporation licenses this file to you under the Apache License,


2025 and LY

…rmeriaHttpServerParser.java Co-authored-by: Trustin Lee <trustin@linecorp.com>

…rmeriaRpcServerParser.java Co-authored-by: Trustin Lee <trustin@linecorp.com>

codecov · 2025-04-10T02:58:02Z

Codecov Report

Attention: Patch coverage is 92.66055% with 8 lines in your changes missing coverage. Please review.

Project coverage is 74.62%. Comparing base (8150425) to head (23843d7).
Report is 78 commits behind head on main.

Files with missing lines	Patch %	Lines
.../server/brave/RpcServiceRequestContextAdapter.java	80.76%	3 Missing and 2 partials ⚠️
...corp/armeria/server/brave/ArmeriaServerParser.java	84.61%	0 Missing and 2 partials ⚠️
...orp/armeria/server/brave/AbstractBraveService.java	95.45%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6115      +/-   ##
============================================
+ Coverage     74.46%   74.62%   +0.15%     
- Complexity    22234    22493     +259     
============================================
  Files          1963     1976      +13     
  Lines         82437    83062     +625     
  Branches      10764    10800      +36     
============================================
+ Hits          61385    61983     +598     
- Misses        15918    15929      +11     
- Partials       5134     5150      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jrhee17 · 2025-05-12T02:45:25Z

Let me go ahead and merge this next week (5/19) unless anyone has further comments

jrhee17 · 2025-05-20T08:13:26Z

Let me go ahead and merge this PR.

minimal impl

037585c

jrhee17 added the new feature label Feb 20, 2025

jrhee17 added this to the 1.33.0 milestone Feb 20, 2025

jrhee17 force-pushed the feat/brave-rpc branch 2 times, most recently from 1c0572c to c53cb0a Compare February 21, 2025 05:39

fix test

7f89976

jrhee17 force-pushed the feat/brave-rpc branch from 73dca92 to 7f89976 Compare February 21, 2025 06:34

lint

45470ae

jrhee17 marked this pull request as ready for review February 21, 2025 08:07

jrhee17 requested review from ikhoon, minwoox and trustin as code owners February 21, 2025 08:07

ikhoon reviewed Feb 24, 2025

View reviewed changes

address comment by @ikhoon

42fa304

ikhoon reviewed Mar 6, 2025

View reviewed changes

jrhee17 added 2 commits April 3, 2025 15:59

Merge branch 'main' into feat/brave-rpc

8fc35e7

address comment by @ikhoon

6d2fb8d

ikhoon approved these changes Apr 7, 2025

View reviewed changes

minwoox approved these changes Apr 8, 2025

View reviewed changes

trustin reviewed Apr 8, 2025

View reviewed changes

jrhee17 and others added 3 commits April 10, 2025 11:36

Merge branch 'main' into feat/brave-rpc

4fd2f99

Update brave/brave6/src/main/java/com/linecorp/armeria/server/brave/A…

df596b5

…rmeriaHttpServerParser.java Co-authored-by: Trustin Lee <trustin@linecorp.com>

Update brave/brave6/src/main/java/com/linecorp/armeria/server/brave/A…

ca8b036

…rmeriaRpcServerParser.java Co-authored-by: Trustin Lee <trustin@linecorp.com>

jrhee17 added 2 commits April 10, 2025 11:59

address comments by @minwoox and @trustin

af242c3

address comment by @minwoox

15f5e09

github-actions Bot added the Stale label May 11, 2025

address comment by @trustin

23843d7

github-actions Bot removed the Stale label May 13, 2025

jrhee17 merged commit 2bc4f17 into line:main May 20, 2025

ikhoon mentioned this pull request Aug 5, 2025

Support BraveRpcService #6084

Closed

		abstract Tracer tracer();

		abstract RequestContextCurrentTraceContext currentTraceContext();

Uh oh!

Conversation

jrhee17 commented Feb 20, 2025

Uh oh!

github-actions Bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: 23843d7)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jrhee17 commented May 12, 2025

Uh oh!

jrhee17 commented May 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Feb 20, 2025 •

edited

Loading

🔍 Build Scan® (commit: `23843d7`)

codecov Bot commented Apr 10, 2025 •

edited

Loading