support apache dubbo asynchronous call #1114 by CodingSinger · Pull Request #1124 · alibaba/Sentinel

CodingSinger · 2019-10-29T12:09:09Z

linlinisme · 2019-10-30T10:54:21Z

        } catch (RpcException e) {
-            Tracer.traceEntry(e, interfaceEntry);
-            Tracer.traceEntry(e, methodEntry);
+            // catch timeout or nonbiz-exception when in sync model
+            trace(e, invocation);
            throw e;
-        } finally {
-            if (methodEntry != null) {
-                methodEntry.exit();
-            }
-            if (interfaceEntry != null) {
-                interfaceEntry.exit();
-            }
        }


interfaceEntry and interfaceEntry exit() method should ensure executed regardless of whether there is an exception?

The logic might be:

When the invocation is successful, then the entries will be exited in onResponse callback.

When the invocation fails, then record the exception and complete the entries. For asynchronous call, it's handled in whenComplete callback.

But for the synchronous case, we may need to check whether the result has exceptions or not, or some business exceptions might be ignored.

Besides, could you please provide a more detailed design in the PR description? @CodingSinger

根据我的理解，在Dubbo2.7.1中，请求如果正常返回，则会回调Filter#onResponse,如果请求超时或者是发生Rpc框架异常，则分两种情况：

当同步调用时，需要自己try catch来捕获。

当异步调用时，异常会在filter丢失，如果需要在filter记录异常，需要自己用whenComplete的方式来回调。

综上，我们需要在三个地方埋点。onResponse的trace针对正常返回，try...catch的trace针对同步调用异常，whenComplete针对异步调用异常

linlinisme · 2019-10-30T10:55:39Z

+
+    static void trace(Throwable throwable, Invocation invocation) {


invocation arg is not used?

是的，暂时没作用，可以去掉

linlinisme · 2019-10-30T11:01:04Z

+            rpcContext.set(DubboUtils.DUBBO_INTERFACE_ENTRY_KEY, interfaceEntry);
+            rpcContext.set(DubboUtils.DUBBO_METHOD_ENTRY_KEY, methodEntry);


this is neccessary?

是的，异步调用的情况下，我们需要保存对应的Entry。

linlinisme · 2019-10-30T11:01:34Z

        } catch (RpcException e) {
-            Tracer.traceEntry(e, interfaceEntry);
-            Tracer.traceEntry(e, methodEntry);
+            trace(e, invocation);
            throw e;
-        } finally {
-            if (methodEntry != null) {
-                methodEntry.exit(1, invocation.getArguments());
-            }
-            if (interfaceEntry != null) {
-                interfaceEntry.exit();
-            }
-            ContextUtil.exit();
        }
    }


miss exit() operate?

trace方法包括了exit的逻辑

sczyh30 · 2019-10-31T08:51:37Z

-                .append(":")
-                .append(invocation.getMethodName())
-                .append("(");
+        buf.append(invoker.getUrl().getEncodedServiceKey())


What's the difference between invoker.getInterface().getName() and invoker.getUrl().getEncodedServiceKey()?

细分粒度吧，getEncodedServiceKey细分为interfaceName+group+version

Friendly ping :)

最近工作有点忙... 没及时响应，抱歉

细分粒度吧，getEncodedServiceKey细分为interfaceName+group+version

This could bring breaking changes, which is not recommended. Actually we've had a discussion before: #67

In most scenarios, the groupName and version are not necessary for flow control. Maybe we could add a property to control it.

我也赞同#67这个人的回答，为啥不把group和version作为resourceName的一部分呢？当用户决定使用group和version属性的时候，往往这也表示这是对同一个java的接口多种实现，难道不应该区分开来吗

We could add a property configuration to control this (e.g. in DubboConfig), but I don't think it's a good idea to change the default behavior, as this could bring breaking changes, which will break existing rules of users.

你是说不兼容的问题吗，确实应该设置一个开关去让用户选择使用getEncodedServiceKey或者是getInterface

sczyh30 · 2019-10-31T08:56:45Z

+        Entry methodEntry = (Entry) RpcContext.getContext().get(DubboUtils.DUBBO_METHOD_ENTRY_KEY);
+        if (methodEntry != null) {
+            Tracer.traceEntry(throwable, methodEntry);
+            methodEntry.exit();


This should be methodEntry.exit(1, args).

args有啥作用吗~

The args is used for parameter flow control. The args in SphU.entry(xxx, et, c, args) and entry.exit(c, args) should match, or statistic error might occur.

好的，我将会传递invocation作为args

所以，同样也需要在获取entry的时候，调用带args的参数？

Yes. You may refer to the former implementation.

sczyh30 · 2019-10-31T08:58:55Z

        return buf.toString();
    }

-    public static String getResourceName(Invoker<?> invoker, Invocation invocation, String prefix) {


Why this is removed? It was added in #859

好吧.. 我代码是从https://github.com/apache/dubbo-sentinel-support/blob/master/src/main/java/com/alibaba/csp/sentinel/adapter/dubbo/DubboUtils.java 拷贝来的，应该是该仓库对该功能的修改未同步过去

We sync the code in every release, while the baseline is in this repo.

sczyh30 · 2019-10-31T09:06:00Z

        } catch (RpcException e) {
-            Tracer.traceEntry(e, interfaceEntry);
-            Tracer.traceEntry(e, methodEntry);
+            // catch timeout or nonbiz-exception when in sync model
+            trace(e, invocation);
            throw e;
-        } finally {
-            if (methodEntry != null) {
-                methodEntry.exit();
-            }
-            if (interfaceEntry != null) {
-                interfaceEntry.exit();
-            }
        }


The logic might be:

When the invocation is successful, then the entries will be exited in onResponse callback.

When the invocation fails, then record the exception and complete the entries. For asynchronous call, it's handled in whenComplete callback.

But for the synchronous case, we may need to check whether the result has exceptions or not, or some business exceptions might be ignored.

Besides, could you please provide a more detailed design in the PR description? @CodingSinger

sczyh30 · 2019-10-31T09:08:05Z

+    }
+
+
+    static void trace(Throwable throwable, Invocation invocation) {


The name (trace) can be confusing. How about traceAndComplete or traceAndExit?

traceException？

In this method the entries are also exited, so the name traceException might not be enough.

嗯，也对，traceAndExit 更合适

sczyh30 · 2019-11-04T05:36:46Z

Friendly ping :)

codecov-io · 2019-11-05T02:15:43Z

Codecov Report

Merging #1124 into master will increase coverage by 0.27%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master    #1124      +/-   ##
============================================
+ Coverage     43.01%   43.29%   +0.27%     
- Complexity     1567     1578      +11     
============================================
  Files           337      338       +1     
  Lines          9877     9892      +15     
  Branches       1332     1335       +3     
============================================
+ Hits           4249     4283      +34     
+ Misses         5099     5082      -17     
+ Partials        529      527       -2

Impacted Files	Coverage Δ	Complexity Δ
...alibaba/csp/sentinel/adapter/dubbo/DubboUtils.java	`95.83% <100%> (+0.37%)`	`9 <1> (+2)`	⬆️
.../sentinel/adapter/dubbo/DubboAppContextFilter.java	`100% <100%> (ø)`	`3 <2> (ø)`	⬇️
...csp/sentinel/adapter/dubbo/config/DubboConfig.java	`80% <60%> (-10%)`	`9 <2> (+2)`
...entinel/adapter/dubbo/BaseSentinelDubboFilter.java	`73.91% <73.91%> (ø)`	`3 <3> (?)`
...nel/adapter/dubbo/SentinelDubboProviderFilter.java	`83.33% <90%> (+24.71%)`	`2 <0> (ø)`	⬇️
...nel/adapter/dubbo/SentinelDubboConsumerFilter.java	`95% <92.3%> (+45%)`	`3 <0> (+1)`	⬆️
...tinel/slots/block/flow/param/ParamFlowChecker.java	`55.4% <0%> (+2.7%)`	`29% <0%> (+1%)`	⬆️
...m/alibaba/csp/sentinel/log/DateFileLogHandler.java	`57.57% <0%> (+3.03%)`	`7% <0%> (+1%)`	⬆️
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java	`100% <0%> (+3.44%)`	`11% <0%> (+1%)`	⬆️

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 49861f4...9d311f2. Read the comment docs.

sczyh30 · 2019-11-19T10:51:30Z

Could you please resolve the conflicts? You may add an additional resourceType when calling SphU.entry(xxx) (see #1142).

CodingSinger · 2019-11-26T01:38:12Z

Could you please resolve the conflicts? You may add an additional resourceType when calling SphU.entry(xxx) (see #1142).

ok，i will resolve it soon.

sczyh30 · 2019-12-02T02:14:14Z

+import org.apache.dubbo.rpc.Result;
+import org.apache.dubbo.rpc.RpcContext;
+
+public abstract class BaseSentinelDubboFilter implements Filter {


Could you please add your @author information here?

sczyh30 · 2019-12-02T02:14:44Z

@@ -0,0 +1,41 @@
+package com.alibaba.csp.sentinel.adapter.dubbo;


Plz add the license header here like:

/* * Copyright 1999-2019 Alibaba Group Holding Ltd. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */

sczyh30 · 2019-12-02T02:38:32Z

+            if (result instanceof AsyncRpcResult) {
+                // catch timeout or nonbiz-exception when in async model
+                AsyncRpcResult asyncRpcResult = (AsyncRpcResult) result;
+                asyncRpcResult.getValueFuture().whenComplete((rs,ex) -> {


This is not compatible with Dubbo 2.7.2+. Since 2.7.2, the AsyncRpcResult directly extends the CompletableFuture and removed the getValueFuture method. You have to handle this for both versions.

sczyh30 · 2019-12-02T03:09:42Z

+    @Override
+    public Result onResponse(Result appResponse, Invoker<?> invoker, Invocation invocation) {
+        //it's unnecessary to traceAndExit the business exception
+        traceAndExit(null, invocation);


Why it's not necessary to trace the biz exception? I've tested three different scenarios:

Normal responses will be handled in the Filter#onResponse callback (whenComplete callback will not catch it)

Timeout. The org.apache.dubbo.remoting.TimeoutException will be caught in the whenComplete callback (Filter#onResponse will not catch it)

Biz exception. Both Filter#onResponse and the whenComplete callback could get signaled, but the Filter#onResponse will be notified first, so if we don't trace the biz exception here, it will be lost. We might need to trace the appResponse.getException() here.

CodingSinger · 2019-12-03T11:53:51Z

Because of some stability and compatibility reasons, we decide to support the Dubbo based on Dubbo version 2.7.3.
Now normal responses and bizException will be handled in the Listener#onResponsecallback.
In addition, we also provide a parameter DubboConfig.TRACE_BIZ_EXCEPTION_ENABLED to let users decide whether to trace the BizException. Users can call the SentinelConfig.setConfig to enable or disable that .
TimeoutException or other RPCException will be handled in the Filter.Listener#onError .

sczyh30 · 2019-12-05T03:42:02Z

+    }
+
+    public static Boolean getDubboBizExceptionTraceEnabled(){
+        return TRUE_STR.equalsIgnoreCase(SentinelConfig.getConfig(TRACE_BIZ_EXCEPTION_ENABLED));


This should be enabled by default.

sczyh30 · 2019-12-05T03:44:01Z

+            RpcContext.getContext().remove(DubboUtils.DUBBO_INTERFACE_ENTRY_KEY);
+        }
+        if (!(interfaceEntry instanceof AsyncEntry)) {
+            ContextUtil.exit();


In the consumer side, we don't need to (and should not) clear the context (though there's guard checking in ContextUtil.exit())

sczyh30 · 2019-12-05T03:47:08Z

-                Tracer.traceEntry(e, interfaceEntry);
-                Tracer.traceEntry(e, methodEntry);
+            if (InvokeMode.SYNC == invokeMode) {
+                interfaceEntry = SphU.entry(interfaceResourceName, ResourceTypeConstants.COMMON_RPC, EntryType.OUT, invocation.getArguments());


The interface entry should not carry the invocation.getArguments(), which is only needed in the method resource.

sry. that's my mistake.

sczyh30 · 2019-12-06T08:44:45Z

-        return String.format("Hello, %s at %s", name, LocalDateTime.now());
+        final AsyncContext asyncContext = RpcContext.startAsync();
+        new Thread(() -> {
+            // 如果要使用上下文，则必须要放在第一句执行


Using English comment is better :)

sry. I will delete this modification.

sczyh30 · 2019-12-06T08:45:43Z

+    public static final String DUBBO_INTERFACE_GROUP_VERSION_ENABLED = "csp.sentinel.dubbo.interface.group.version.enabled";

-    public static boolean isUsePrefix(){
+    public static final String TRACE_BIZ_EXCEPTION_ENABLED = "csp.sentinel.dubbo.trace.biz.exceptio.enabled";


sczyh30 · 2019-12-06T08:46:19Z

        } catch (RpcException e) {
-            Tracer.traceEntry(e, interfaceEntry);
-            Tracer.traceEntry(e, methodEntry);
            throw e;


Missing traceAndExit() here?

No. It's also unnecessary to catch the RpcException in SentinelDubboProvider and SentinelDubboConsumer to trace. Dubbo will catch the Exception when exec filter.invoke and call the onError to handle this.

sczyh30 · 2019-12-06T12:29:18Z

+            interfaceEntry = SphU.entry(interfaceResourceName, ResourceTypeConstants.COMMON_RPC, EntryType.IN);
+            methodEntry = SphU.entry(methodResourceName, ResourceTypeConstants.COMMON_RPC, EntryType.IN, invocation.getArguments());
+            rpcContext.set(DubboUtils.DUBBO_INTERFACE_ENTRY_KEY, interfaceEntry);
+            rpcContext.set(DubboUtils.DUBBO_METHOD_ENTRY_KEY, methodEntry);


A fatal bug here: we have two resources here: interface level and method level. If we set a flow rule for the method resource and when it's blocked, the Entry of the interface level will not be carried in the RpcContext, leading to unexpected behavior (some entries cannot be exited finally). This should be carefully designed and tested. More unit tests and integration tests are needed.

Yes, the entry should be bound to context immediately when finish the call of entry method. And should call the superclass's traceAndExit if there is a BlockException be thrown. I will write some tests for this.~

sczyh30

LGTM

sczyh30 · 2019-12-23T09:46:00Z

Nice work. Thanks for contributing!

…ibaba#1124) * Improve async support for Dubbo 2.7.2 and above (not compatible with 2.7.0 and 2.7.1 due to the bad compatibility design of Dubbo Filter)

sczyh30 added kind/enhancement Category issues or prs related to enhancement. to-review To review labels Oct 29, 2019

linlinisme reviewed Oct 30, 2019

View reviewed changes

sczyh30 requested changes Oct 31, 2019

View reviewed changes

sczyh30 reviewed Oct 31, 2019

View reviewed changes

CodingSinger force-pushed the sentinel-dubbo branch from 03520a3 to 08742d6 Compare November 5, 2019 02:07

CodingSinger force-pushed the sentinel-dubbo branch from 08742d6 to bf4a611 Compare November 26, 2019 01:48

sczyh30 added the area/integrations Issues or PRs related to integrations with open-source components label Dec 2, 2019

sczyh30 reviewed Dec 2, 2019

View reviewed changes

CodingSinger force-pushed the sentinel-dubbo branch from bf4a611 to 92912d2 Compare December 2, 2019 02:42

sczyh30 requested changes Dec 2, 2019

View reviewed changes

CodingSinger force-pushed the sentinel-dubbo branch from 92912d2 to a847403 Compare December 3, 2019 11:40

sczyh30 reviewed Dec 5, 2019

View reviewed changes

CodingSinger force-pushed the sentinel-dubbo branch 2 times, most recently from eb73dd3 to 4853f56 Compare December 5, 2019 09:47

sczyh30 reviewed Dec 6, 2019

View reviewed changes

CodingSinger force-pushed the sentinel-dubbo branch from 4853f56 to 0b402bf Compare December 6, 2019 09:12

support apache dubbo asynchronous call alibaba#1114

9d311f2

CodingSinger force-pushed the sentinel-dubbo branch from 0b402bf to 9d311f2 Compare December 6, 2019 09:15

sczyh30 requested changes Dec 6, 2019

View reviewed changes

add unit test and it test

db8a612

CodingSinger force-pushed the sentinel-dubbo branch from acddddd to db8a612 Compare December 10, 2019 07:42

sczyh30 approved these changes Dec 23, 2019

View reviewed changes

sczyh30 merged commit 47100e6 into alibaba:master Dec 23, 2019

sczyh30 removed the to-review To review label Dec 23, 2019

sczyh30 mentioned this pull request Dec 23, 2019

this extension can't work well when consumer in async mode apache/dubbo-sentinel-support#1

Open

sczyh30 added the size/XL Indicate a PR that changes 500-999 lines. label Dec 23, 2019

sczyh30 mentioned this pull request Feb 12, 2020

sentinel-apache-dubbo-adapter 1.7.1 does not work for Dubbo 2.7.1 #1280

Closed

sczyh30 mentioned this pull request Apr 26, 2020

[1416] fix sentinel-apache-dubbo-adapter full GC problem #1431

Merged


		static void trace(Throwable throwable, Invocation invocation) {

		rpcContext.set(DubboUtils.DUBBO_INTERFACE_ENTRY_KEY, interfaceEntry);
		rpcContext.set(DubboUtils.DUBBO_METHOD_ENTRY_KEY, methodEntry);

		@@ -0,0 +1,41 @@
		package com.alibaba.csp.sentinel.adapter.dubbo;

Uh oh!

Conversation

CodingSinger commented Oct 29, 2019 • edited by sczyh30 Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

sczyh30 commented Nov 4, 2019

Uh oh!

codecov-io commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sczyh30 commented Nov 19, 2019

Uh oh!

CodingSinger commented Nov 26, 2019

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

CodingSinger commented Oct 29, 2019 •

edited by sczyh30

Loading

codecov-io commented Nov 5, 2019 •

edited

Loading

CodingSinger commented Dec 3, 2019 •

edited

Loading