[Fix-16929][Meter] Fix startup failed when metrics is disabled. #17094
Conversation
yingh0ng
requested review from
ruanwenjun,
EricGao888 and
Gallardot
as code owners
| @@ -42,20 +43,28 @@ | |||
| */ | |||
| @Configuration(proxyBeanMethods = false) | |||
| @EnableAspectJAutoProxy | |||
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") | |||
There was a problem hiding this comment.
Can we remove this config? Then we will enable the metrics in all times.
There was a problem hiding this comment.
Do you mean, users are not allowed to disable metrics?
There was a problem hiding this comment.
Can we remove this config? Then we will enable the metrics in all times.
LGTM. Enabling metric is a better practice, but why do users need to turn it off?
There was a problem hiding this comment.
Do you mean, users are not allowed to disable metrics?
Yes, the server heartbeat rely on the metrics provider, so this is must.
There was a problem hiding this comment.
Yes, the server heartbeat rely on the metrics provider, so this is must.
LGTM. Fix it later.
| public class MeterAutoConfiguration { | ||
|
|
||
| @Bean | ||
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") |
There was a problem hiding this comment.
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") |
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "false", matchIfMissing = true) | ||
| public EmptyMetricsProvider emptyMetricsProvider() { | ||
| return new EmptyMetricsProvider(); | ||
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") |
There was a problem hiding this comment.
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "false", matchIfMissing = true) | |
| public EmptyMetricsProvider emptyMetricsProvider() { | |
| return new EmptyMetricsProvider(); | |
| } | |
| @Bean | |
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") |
| public TimedAspect timedAspect(MeterRegistry registry) { | ||
| return new TimedAspect(registry); | ||
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") |
There was a problem hiding this comment.
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") |
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You 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. | ||
| */ | ||
|
|
||
| package org.apache.dolphinscheduler.meter.metrics; | ||
|
|
||
| public class EmptyMetricsProvider implements MetricsProvider { | ||
|
|
||
| @Override | ||
| public SystemMetrics getSystemMetrics() { | ||
| return new SystemMetrics(); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
| /* | |
| * Licensed to the Apache Software Foundation (ASF) under one or more | |
| * contributor license agreements. See the NOTICE file distributed with | |
| * this work for additional information regarding copyright ownership. | |
| * The ASF licenses this file to You 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. | |
| */ | |
| package org.apache.dolphinscheduler.meter.metrics; | |
| public class EmptyMetricsProvider implements MetricsProvider { | |
| @Override | |
| public SystemMetrics getSystemMetrics() { | |
| return new SystemMetrics(); | |
| } | |
| } |
| @@ -42,20 +43,28 @@ | |||
| */ | |||
| @Configuration(proxyBeanMethods = false) | |||
| @EnableAspectJAutoProxy | |||
| @ConditionalOnProperty(prefix = "metrics", name = "enabled", havingValue = "true") | |||
There was a problem hiding this comment.
Do you mean, users are not allowed to disable metrics?
Yes, the server heartbeat rely on the metrics provider, so this is must.
|
@ruanwenjun The OWASP dependency check failed. PTAL.
|
You don't need to pay attention to it at present. Just ignore it. |
|
Purpose of the pull request
close #16929
Brief change log
Verify this pull request
This pull request is already covered by existing tests, such as (please describe tests).
Pull Request Notice
Pull Request Notice
If your pull request contains incompatible change, you should also add it to
docs/docs/en/guide/upgrade/incompatible.md