Improve the logs when the heartbeat response indicates failure #1303

linlinisme · 2020-02-24T10:22:54Z

Describe what this PR does / why we need it

Does this pull request fix one issue?

fix #1194

Describe how you did it

Describe how to verify it

Special notes for reviews

using the LongAdder rather than AtomicInteger to Provides better performance

merge master

…perties directory

merge online

…cator to load log config

merge onlie

merge online

…x-issue62

This reverts commit d401c2f

merge online

merge remote master

merge online

codecov-io · 2020-02-24T10:37:11Z

Codecov Report

Merging #1303 into master will decrease coverage by 0.02%.
The diff coverage is 40.27%.

@@             Coverage Diff              @@
##             master    #1303      +/-   ##
============================================
- Coverage     42.74%   42.71%   -0.03%     
+ Complexity     1604     1603       -1     
============================================
  Files           344      344              
  Lines         10186    10186              
  Branches       1380     1380              
============================================
- Hits           4354     4351       -3     
- Misses         5287     5290       +3     
  Partials        545      545

Impacted Files	Coverage Δ	Complexity Δ
.../sentinel/adapter/dubbo/DubboAppContextFilter.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
...erver/envoy/rls/flow/SimpleClusterFlowChecker.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...inel/adapter/gateway/zuul/filters/EntryHolder.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...a/csp/sentinel/slots/HotParamSlotChainBuilder.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...sp/sentinel/slots/system/SystemStatusListener.java	`37.03% <0%> (ø)`	`3 <0> (ø)`	⬇️
.../rls/datasource/EnvoyRlsRuleDataSourceService.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...apter/gateway/zuul/filters/SentinelEntryUtils.java	`13.63% <0%> (ø)`	`1 <0> (ø)`	⬇️
...a/csp/sentinel/datasource/etcd/EtcdDataSource.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...er/gateway/zuul/filters/SentinelZuulPreFilter.java	`14.28% <0%> (ø)`	`3 <0> (ø)`	⬇️
...com/alibaba/csp/sentinel/log/CommandCenterLog.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
... and 117 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 7a4391d...d39d090. Read the comment docs.

sczyh30 · 2020-02-24T14:49:48Z

#926 brought some conflicts. Could you please resolve it?

...tty-http/src/main/java/com/alibaba/csp/sentinel/transport/heartbeat/HttpHeartbeatSender.java

Merge remote master

cdfive · 2020-02-24T15:51:40Z

...tty-http/src/main/java/com/alibaba/csp/sentinel/transport/heartbeat/HttpHeartbeatSender.java

+            RecordLog.warn("[HttpHeartbeatSender] Failed to send heartbeat to "
+                    + consoleHost + ":" + consolePort + ", response : ", response);


Here we may need use placeholder for log: RecordLog.warn("xxx{},{}", arg1,arg2);.
Or just concat all message string.

Besides, I found that SimpleHttpResponse#toString may throws NPE, when body is null,
In SimpleHttpResponse#108L, maybe need add checking logic.

Yes. Maybe just logging the response status code is enough?

emm. I check the response Headers. It is a supplement of the server version、location 、name, may be it is not necessary to print.

How about else branch, do we need to add some log? The NPE remains in SimpleHttpResponse#getBodyAsString() at line 108, when body is null it throws.

emm.. it seems there more than one point need to handle the NPE

1.com.alibaba.csp.sentinel.transport.heartbeat.client.SimpleHttpResponseParser#parse

2.com.alibaba.csp.sentinel.transport.heartbeat.client.SimpleHttpResponse#parseCharset

3.com.alibaba.csp.sentinel.transport.heartbeat.client.SimpleHttpResponse#getBodyAsString、 return new String(body, charset);

any suggestion?

I think that 3 need to check.To verify it, we can throw Exception in MachineRegistryController#receiveHeartBeat of dashboard, and call SimpleHttpResponse#toString().

I think that 3 need to check.To verify it, we can throw Exception in MachineRegistryController#receiveHeartBeat of dashboard, and call SimpleHttpResponse#toString().

It still will return http 200 ok response code, even though throw ex. It seems SimpleHttpResponseParser don't check the response data, which may need to be checked. How about we open another issue to discuss ?

I think that 3 need to check.To verify it, we can throw Exception in MachineRegistryController#receiveHeartBeat of dashboard, and call SimpleHttpResponse#toString().

It still will return http 200 ok response code, even though throw ex. It seems SimpleHttpResponseParser don't check the response data, which may need to be checked. How about we open another issue to discuss ?

Yeah, we could open another issue to discuss how to improve it :)

Yes, one issue one PR is clean and consistent.

It still will return http 200 ok response code, even though throw ex. It seems SimpleHttpResponseParser don't check the response data
I got http 500 and the SimpleHttpResponseParser checked the response.

Add this in MachineRegistryController#receiveHeartBeat

if (true) { throw new RuntimeException("test"); }

Then add in SimpleHttpHeartbeatSender#sendHeartbeat

try { System.out.println(response); } catch (Exception e) { e.printStackTrace(); }

Then the response code 500 got, and NPE occured.

…problem

linlinisme · 2020-02-26T06:47:22Z

friendly ping :)

cdfive

LGTM

sczyh30

LGTM

sczyh30 · 2020-03-04T15:39:01Z

Thanks!

…ba#1303)

…in the same process can trace only one (alibaba#1275) (alibaba#1303) * fix trace problem when multi produce/consumer in the same process * uniform parameter manner * variable rename * consumer groups may be same with the producer group Co-authored-by: zhengwen zhu <ahuazhu@gmail.com>

Lin.Liang and others added 30 commits May 9, 2019 14:54

com.alibaba.csp.sentinel.node.StatisticNode#curThreadNum

11dba5e

using the LongAdder rather than AtomicInteger to Provides better performance

reformat the code according to Alibaba Java Coding Guideline

362f326

reformat the code according to Alibaba Java Coding Guideline

50d2a5a

Merge pull request #1 from alibaba/master

509ecf2

merge master

Remove the one redundant space....

01bda6b

fix issue 62 : Support customized log directory and configuration pro…

ce717e9

…perties directory

use System.getProperty to get the property

b8e66b8

Merge pull request #2 from alibaba/master

5c6600d

merge online

merge master latest code

9999154

using SentinelConfigLocator to load sentinel-core config, LogConfigLo…

8f9339b

…cator to load log config

Merge pull request #3 from alibaba/master

2f485b4

merge onlie

merge remote master

c6dd24d

fix typo

810d40a

add null judge

3f98b62

Reorganize the code

23244e9

Reorganize the code

bd8a481

add config test cases

e6bbf79

ConfigUtil.loadPropertiesFromFile when file not exist return null

c91b963

rename classname and method name

d401c2f

rename classname and method name

85173a5

using File.separator to replace "/"

4510234

support retrieve properties from classpath file

c068edd

support retrieve properties from classpath file

9af06c9

add feature support : retrieve properties from relative file

014cda3

reformat code

a899dc6

Merge pull request #4 from alibaba/master

419d97f

merge online

Merge remote-tracking branch 'remotes/origin/master' into 20190531-fi…

80f283e

…x-issue62

Revert "rename classname and method name"

14a1d5b

This reverts commit d401c2f

add copy right doc

ea18020

reset commit

c562474

liangl and others added 7 commits July 6, 2019 12:10

remove unused code

c71759f

merge alibaba master

d387b47

Merge pull request #6 from alibaba/master

56d5df0

merge online

Merge pull request #7 from alibaba/master

22cafd2

merge remote master

Merge pull request #8 from alibaba/master

7a0c6b7

merge online

Improve the logs when the heartbeat response indicates failure (#1194)

7a4391d

Merge branch 'master' into log-heartbeat-response

654d73a

sczyh30 added kind/enhancement Category issues or prs related to enhancement. to-review To review labels Feb 24, 2020

sczyh30 added this to the 1.7.2 milestone Feb 24, 2020

sczyh30 reviewed Feb 24, 2020

View reviewed changes

...tty-http/src/main/java/com/alibaba/csp/sentinel/transport/heartbeat/HttpHeartbeatSender.java Outdated Show resolved Hide resolved

linlinisme and others added 2 commits February 24, 2020 22:59

Merge pull request #9 from alibaba/master

3b0f07d

Merge remote master

HttpHeartbeatSender remove duplicate host port check

f3a32dd

cdfive reviewed Feb 24, 2020

View reviewed changes

simply HeartBeatSender log and fix SimpleHttpResponse.toString() NPE …

d39d090

…problem

cdfive approved these changes Mar 4, 2020

View reviewed changes

sczyh30 approved these changes Mar 4, 2020

View reviewed changes

sczyh30 merged commit 72bfe87 into alibaba:master Mar 4, 2020

sczyh30 removed the to-review To review label Mar 4, 2020

sczyh30 mentioned this pull request Jun 16, 2020

transport module SimpleHttpClient 400 but no any logs #1551

Closed

hughpearse pushed a commit to hughpearse/Sentinel that referenced this pull request Jun 2, 2021

Improve the logs when the heartbeat response indicates failure (aliba…

3b17a32

…ba#1303)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the logs when the heartbeat response indicates failure #1303

Improve the logs when the heartbeat response indicates failure #1303

linlinisme commented Feb 24, 2020

codecov-io commented Feb 24, 2020 •

edited

Loading

sczyh30 commented Feb 24, 2020

cdfive Feb 24, 2020

sczyh30 Feb 24, 2020

linlinisme Feb 25, 2020

cdfive Feb 26, 2020

linlinisme Feb 27, 2020 •

edited

Loading

linlinisme Mar 2, 2020

cdfive Mar 2, 2020

linlinisme Mar 2, 2020

sczyh30 Mar 2, 2020

cdfive Mar 3, 2020

linlinisme commented Feb 26, 2020

cdfive left a comment

sczyh30 left a comment

sczyh30 commented Mar 4, 2020

		RecordLog.warn("[HttpHeartbeatSender] Failed to send heartbeat to "
		+ consoleHost + ":" + consolePort + ", response : ", response);

Improve the logs when the heartbeat response indicates failure #1303

Improve the logs when the heartbeat response indicates failure #1303

Conversation

linlinisme commented Feb 24, 2020

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

codecov-io commented Feb 24, 2020 • edited Loading

Codecov Report

sczyh30 commented Feb 24, 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

linlinisme Feb 27, 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

linlinisme commented Feb 26, 2020

cdfive left a comment

Choose a reason for hiding this comment

sczyh30 left a comment

Choose a reason for hiding this comment

sczyh30 commented Mar 4, 2020

codecov-io commented Feb 24, 2020 •

edited

Loading

linlinisme Feb 27, 2020 •

edited

Loading