[feature] Expose jmx endpoint for sentinel #1947

brotherlu-xcq · 2021-01-06T04:54:58Z

Describe what this PR does / why we need it

expose the metric in Sentinel as jmx bean, which can make the Sentinel metric collected by popular monitor tools like prometheus.

Does this pull request fix one issue?

Fixs #1814

Describe how you did it

Describe how to verify it

visit the resource which control by Sentinel, the open your JMX tool. you will find the metric info like below.

the root type is Sentinel. the second type is your sentinel application name, the JMX bean name is you resource name. the metric data is same with the MetricNode which is writed into metric file.

Special notes for reviews

…com/brothelul/Sentinel into feature-alibaba#1848 � Conflicts: � sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/block/flow/FlowRuleManager.java

cdfive · 2021-01-17T03:49:32Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/block/flow/FlowRuleManager.java

-    private static final ScheduledExecutorService SCHEDULER = Executors.newScheduledThreadPool(1,
+    private static final ScheduledExecutorService SCHEDULER = Executors.newScheduledThreadPool(2,
        new NamedThreadFactory("sentinel-metrics-record-task", true));

    static {
        flowRules.set(Collections.<String, List<FlowRule>>emptyMap());
        currentProperty.addListener(LISTENER);
        startMetricTimerListener();
+        SCHEDULER.scheduleAtFixedRate(new MetricBeanTimerListener(), 0, 1, TimeUnit.SECONDS);
    }


Now there are two thread to execute, node.metrics() is called separately in MetricBeanTimerListener and MetricTimerListener, is there data inaccuracy in multithreading?
Tried with FlowQpsDemo, the PassQps,BlockQps are always 0 in jconsole.

com.alibaba.csp.sentinel.node.metric.MetricWriter#write
com.alibaba.csp.sentinel.node.metric.jmx.MetricBeanWriter#write
When the two operations are put in a same thread, it works fine and got the expected result in jconsole.

Maybe that's the reason, as the comment mentioned in com.alibaba.csp.sentinel.node.StatisticNode#metrics,
The fetch operation is thread-safe under a single-thread scheduler pool.

@Override public Map<Long, MetricNode> metrics() { // The fetch operation is thread-safe under a single-thread scheduler pool. long currentTime = TimeUtil.currentTimeMillis(); currentTime = currentTime - currentTime % 1000; ... return metrics; }

thank you to review my code, I will check the codes and sovle it.

brotherlu-xcq · 2021-01-19T01:55:32Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/node/StatisticNode.java

@@ -113,7 +113,7 @@
    private long lastFetchTime = -1;

    @Override
-    public Map<Long, MetricNode> metrics() {
+    public synchronized Map<Long, MetricNode> metrics() {


@cdfive because the two timer have different bussiness scenes, so I am not put them in one timer and add the synchronized to ensure the thread-safe. could you please help to review this when you you are free? I ran the local test and can find the data in jvm tool.

I run the FlowQpsDemo, and start jconsole, still can't find Sentinel node in jconsole UI. Then I make a breakpoint at com.alibaba.csp.sentinel.node.metric.jmx.MetricBeanTimerListener#aggregate, never entered.
Then make another breakapoint at com.alibaba.csp.sentinel.node.metric.MetricTimerListener#aggregate,
it entered, and resume running, the first breakpoint entered, and Sentinel node appeared, but the the PassQps,BlockQps are always 0.

I wonder that once the StatisticNode#metrics have been invoked, the data will be changed.

If comment the startMetricBeanTimerListener out and merge the logic of MetricBeanTimerListener#run into MetricTimerListener#run and only invoke StatisticNode#metrics once, can get the expected result in jconsole.

could you please try the demo and give some help? @sczyh30 @brothelul

I will try the demo and re-produce the problem you met again, thank you so much!

brotherlu-xcq · 2021-01-21T09:58:49Z

...nel-core/src/main/java/com/alibaba/csp/sentinel/node/metric/jmx/MetricBeanTimerListener.java

@@ -40,11 +40,12 @@ public void run() {

    private List<MetricNode> getLastMetrics(ClusterNode node) {
        final long currentTime = TimeUtil.currentTimeMillis();
-        final long minTime = currentTime - currentTime % 1000;
+        final long maxTime = currentTime - currentTime % 1000;
+        final long minTime = maxTime - 1000;
        return node.rawMetricsInMin(new Predicate<Long>() {


@cdfive I change the poolSize of SCHEDULER into 1, so the two listener will run one by one in thread-safe. And I use the method rawMetricsInMin instead of metrics to get the metrics in last sencod so that the JMX will be same as metrics in file.

@cdfive any idea about this?

@brothelul Nice work! I can get the expected result in jconsole now.

cdfive · 2021-02-02T01:57:04Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/block/flow/FlowRuleManager.java

+    /**
+     * start the MetricBeanTimerListener at the fix rate, the initialDelay is different with the {@link #startMetricTimerListener()},
+     * so that the two task will start running at different time.
+     */
+    private static void startMetricBeanTimerListener() {
+        SCHEDULER.scheduleAtFixedRate(new MetricBeanTimerListener(), 0, 1, TimeUnit.SECONDS);
+    }
+


I found that we can config the interval with csp.sentinel.metric.flush.interval in startMetricTimerListener,
but startMetricBeanTimerListener has one second fixed interval. Do we need to make it configurable? At present, this function is enabled by default, maybe some users want to disable it.

yes, that's a good idea! I will change it later.

cdfive · 2021-02-02T07:32:20Z

Nice work! With this jmx endpoint, it will be helpful when collecting monitor data.
Any other idea? @sczyh30 @jasonjoo2010

BTW, the issue which this PR is to fix seems not corresponding. Maybe another issue? @brothelul

Does this pull request fix one issue?
Fixs #1848

brotherlu-xcq · 2021-02-03T01:43:04Z

yes, it's #1814, not #1848. I changed it.

tianshuang · 2021-05-26T09:35:47Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/node/metric/jmx/MetricBeanWriter.java

+        Set<String> existResource = new HashSet<>();
+        // set or update the new value
+        for (MetricNode metricNode : map.values()) {
+            final String mBeanName = "Sentinel:type=" + appName + ",name=\"" + metricNode.getResource() +"\"";


It is recommended to add classification here, so that Prometheus can search according to classification.
ex:
final String mBeanName = "Sentinel:type=" + appName + ",name=\"" + metricNode.getResource() +"\"" + ",classification=\"" + metricNode.getClassification() +"\"";

@sczyh30 @cdfive Do you think so? I am not clear about the work of Prometheus?

It has been compiled by me for production environment. 😄 You can refer to the mbean of tomcat, which contains the port number.

tianshuang · 2021-05-26T09:36:42Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/node/metric/jmx/MetricBeanWriter.java

+            return;
+        }
+        for (MetricBean metricBean : metricBeans) {
+            final String mBeanName = "Sentinel:type=" + appName + ",name=\"" + metricBean.getResource() +"\"";


Same here as above.
ex:
final String mBeanName = "Sentinel:type=" + appName + ",name=\"" + metricBean.getResource() +"\"" + ",classification=\"" + metricBean.getClassification() +"\"";

sczyh30 · 2021-06-24T06:22:06Z

Nice work. IMHO, how about make it an extension module (users could enable it by just adding a dependency of the extension module)? This could decouple metrics exporters from FlowRuleManager and core (actually initializing it in static block of FlowRuleManager is not a good design...)

sczyh30 · 2021-06-24T06:22:56Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/block/flow/FlowRuleManager.java

    @SuppressWarnings("PMD.ThreadPoolCreationRule")
    private static final ScheduledExecutorService SCHEDULER = Executors.newScheduledThreadPool(1,
        new NamedThreadFactory("sentinel-metrics-record-task", true));

    static {
        flowRules.set(Collections.<String, List<FlowRule>>emptyMap());
        currentProperty.addListener(LISTENER);
+        startMetricBeanTimerListener();


Maybe we could initialize it via InitFunc SPI mechanism?

brotherlu-xcq · 2021-06-24T06:35:44Z

Nice work. IMHO, how about make it an extension module (users could enable it by just adding a dependency of the extension module)? This could decouple metrics exporters from FlowRuleManager and core (actually initializing it in static block of FlowRuleManager is not a good design...)

I am really agree with you that export the metric function to other module instead of in core module. I want still work on this feature, and make a design first about it.

…ture-alibaba#1848

brotherlu-xcq · 2021-07-01T11:40:09Z

This function need some change after we discussed. please do not merge it before I re-submit the new implement.

sczyh30 · 2021-09-15T02:06:36Z

#2275 has been merged

brotherlu-xcq added 7 commits December 31, 2020 18:16

expose the MetricNode AS MBean

9f296ef

add the notes

a163bac

expose the MetricNode as the jmx bean

c78ef02

Merge branches 'feature-alibaba#1848' and 'master' of https://github.…

024241d

…com/brothelul/Sentinel into feature-alibaba#1848 � Conflicts: � sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/block/flow/FlowRuleManager.java

integration test for MBean

afb0123

rename the class and add the notes

cc4a7d0

perfect the notes

3d55d71

sczyh30 added the area/metrics Issues or PRs related to metrics and monitoring label Jan 6, 2021

sczyh30 requested review from zhaoyuguang, a team, sczyh30 and jasonjoo2010 January 6, 2021 06:53

sczyh30 added the kind/feature Category issues or prs related to feature request. label Jan 15, 2021

sczyh30 requested a review from cdfive January 15, 2021 15:20

cdfive reviewed Jan 17, 2021

View reviewed changes

brotherlu-xcq added 2 commits January 18, 2021 20:20

solve the thread-safe issue

fd1c3f7

add notes in MetricMXBean

840f9fc

brotherlu-xcq commented Jan 19, 2021

View reviewed changes

current the collect issue of JMX

72e4604

brotherlu-xcq mentioned this pull request Jan 21, 2021

metrics support? #1975

Closed

brotherlu-xcq added 2 commits January 21, 2021 17:49

fix the blockQps data issue

d7e80e2

change the unit of period in metricMbeanListener

3316cd3

brotherlu-xcq commented Jan 21, 2021

View reviewed changes

brotherlu-xcq added 2 commits January 22, 2021 17:51

add the MBean register log

752b697

add Listener log

5c49633

cdfive reviewed Feb 2, 2021

View reviewed changes

make the mbean listener flush interval configurable

46eb451

sczyh30 added this to the 1.8.2 milestone Feb 3, 2021

tianshuang reviewed May 26, 2021

View reviewed changes

sczyh30 reviewed Jun 24, 2021

View reviewed changes

sczyh30 modified the milestones: 1.8.2, 1.8.3 Jun 28, 2021

Merge branch 'master' of https://github.com/alibaba/Sentinel into fea…

0977c0a

…ture-alibaba#1848

brotherlu-xcq mentioned this pull request Jul 2, 2021

[feature] Expose jmx endpoint for sentinel. #2275

Merged

sczyh30 removed this from the 1.8.3 milestone Sep 15, 2021

sczyh30 closed this Sep 15, 2021

brotherlu-xcq deleted the feature-#1848 branch September 15, 2021 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Expose jmx endpoint for sentinel #1947

[feature] Expose jmx endpoint for sentinel #1947

brotherlu-xcq commented Jan 6, 2021 •

edited

Loading

cdfive Jan 17, 2021 •

edited

Loading

brotherlu-xcq Jan 18, 2021

brotherlu-xcq Jan 19, 2021

cdfive Jan 19, 2021 •

edited

Loading

brotherlu-xcq Jan 19, 2021

brotherlu-xcq Jan 21, 2021

brotherlu-xcq Jan 27, 2021

cdfive Feb 2, 2021

cdfive Feb 2, 2021

brotherlu-xcq Feb 2, 2021

cdfive commented Feb 2, 2021

brotherlu-xcq commented Feb 3, 2021

tianshuang May 26, 2021

brotherlu-xcq Jun 2, 2021

tianshuang Jun 2, 2021

tianshuang May 26, 2021

sczyh30 commented Jun 24, 2021

sczyh30 Jun 24, 2021

brotherlu-xcq commented Jun 24, 2021

brotherlu-xcq commented Jul 1, 2021

sczyh30 commented Sep 15, 2021

[feature] Expose jmx endpoint for sentinel #1947

[feature] Expose jmx endpoint for sentinel #1947

Conversation

brotherlu-xcq commented Jan 6, 2021 • edited Loading

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

cdfive Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdfive Jan 19, 2021 • 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

cdfive commented Feb 2, 2021

brotherlu-xcq commented Feb 3, 2021

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

sczyh30 commented Jun 24, 2021

Choose a reason for hiding this comment

brotherlu-xcq commented Jun 24, 2021

brotherlu-xcq commented Jul 1, 2021

sczyh30 commented Sep 15, 2021

brotherlu-xcq commented Jan 6, 2021 •

edited

Loading

cdfive Jan 17, 2021 •

edited

Loading

cdfive Jan 19, 2021 •

edited

Loading