JAV-526 Writing metric data into files #300
Conversation
zhengyangyong
force-pushed
the
JAV-526
branch
from
dfdf4e0 to
2c41cbd
Compare
| public static final String METRICS_POLL_TIME = "servicecomb.metrics.polltime"; | ||
| public static final String METRICS_FILE_ENABLED = "servicecomb.metrics.file.enabled"; | ||
| public static final String METRICS_FILE_PATH = "servicecomb.metrics.file.file_root_path"; | ||
| public static final String METRICS_FILE_SIZE = "servicecomb.metrics.file.max_rolling_size"; |
There was a problem hiding this comment.
The constant variable should be meaningful, Please add the important information about "rolling" to the constant.
| public static final String METRICS_FILE_ENABLED = "servicecomb.metrics.file.enabled"; | ||
| public static final String METRICS_FILE_PATH = "servicecomb.metrics.file.file_root_path"; | ||
| public static final String METRICS_FILE_SIZE = "servicecomb.metrics.file.max_rolling_size"; | ||
| public static final String METRICS_FILE_COUNT = "servicecomb.metrics.file.max_rolling_count"; |
| public void updateImpl(List<Metric> metrics) { | ||
| Preconditions.checkNotNull(metrics, "metrics"); | ||
| //这些参数是一次一起计算的，所以不需要将它们转化为独立的Metric，直接取值输出 | ||
| Map<String, String> queueMetrics = metricsRegistry.calculateQueueMetrics(); |
There was a problem hiding this comment.
Single Responsibility！！！ Do not get the metrics here.
There was a problem hiding this comment.
add MetricsContentConvertor interface to convert metrics and add MetricsContentFormatter interface to format output style
| private final MetricsServoRegistry metricsRegistry; | ||
| private final MetricsFileOutput metricsOutput; | ||
|
|
||
| public SeparatedMetricObserver(String observerName, MetricsFileOutput metricsOutput, |
There was a problem hiding this comment.
What's the meaning of Separated?
| InetAddress localHost = InetAddress.getLocalHost(); | ||
| hostName = localHost.getHostName(); | ||
| } catch (UnknownHostException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Using the logger to log the exception. The UnknownHost is not a good solution here, we can still use IP address as the HostName.
There was a problem hiding this comment.
NetUtils already have this information?
| Microservice microservice = RegistryUtils.getMicroservice(); | ||
| applicationName = String.join(".", microservice.getAppId(), microservice.getServiceName()); | ||
| } else { | ||
| applicationName = String.join(".", hostName, "test"); |
There was a problem hiding this comment.
What's the mean of test, if you just want to make the UT passed, it's not an acceptable reason.
| fileAppender.setLogPermission("rw-------"); | ||
| fileAppender.setFile(finalPath); | ||
| fileAppender.setLayout(new PatternLayout("%m%n")); | ||
| fileAppender.setThreshold(Priority.FATAL); |
There was a problem hiding this comment.
Why do you chose the FATAL?
| try { | ||
| SeparatedOutputData outputData = new SeparatedOutputData(this.applicationName, hostName, metricName, | ||
| metrics.get(metricName)); | ||
| event = new LoggingEvent(fileName, Category.getInstance(fileName), Priority.FATAL, |
There was a problem hiding this comment.
Don't use the deprecated method here.
| event = new LoggingEvent(fileName, Category.getInstance(fileName), Priority.FATAL, | ||
| mapper.writeValueAsString(outputData), null); | ||
| } catch (JsonProcessingException e) { | ||
| logger.error("parse metric data error"); |
There was a problem hiding this comment.
Do not eating the exception!!!
| } | ||
|
|
||
| class SeparatedOutputData { | ||
| private String plugin_id = null; |
There was a problem hiding this comment.
Using the camel case for none constant variable.
|
|
||
| class SeparatedOutputData { | ||
| private String plugin_id = null; | ||
| private Map<String, Object> metric = null; |
There was a problem hiding this comment.
This metric filed is useless, please remove it.
There was a problem hiding this comment.
it's used for json serialize
| configuration.setProperty(MetricsFileOutput.METRICS_POLL_TIME, 1); | ||
| configuration.setProperty(MetricsFileOutput.METRICS_FILE_ENABLED, true); | ||
|
|
||
| // configuration.setProperty(MetricsFileOutput.METRICS_FILE_PATH, "D:/Temp"); |
|
|
||
| DynamicPropertyFactory.initWithConfigurationSource(configuration); | ||
| MetricsServoRegistry.metricsList.clear(); | ||
| MetricsServoRegistry.LOCAL_METRICS_MAP = new ThreadLocal<>(); |
There was a problem hiding this comment.
I don't like this static variable.
| Microservice microservice = RegistryUtils.getMicroservice(); | ||
| fileNameHeader = String.join(".", microservice.getAppId(), microservice.getServiceName()); | ||
| } else { | ||
| fileNameHeader = String.join(".", "local", "test"); |
There was a problem hiding this comment.
Throw the exception here, as we cannot find the microservice information.
| private final ObjectMapper mapper = new ObjectMapper(); | ||
|
|
||
| @Override | ||
| public Map<String, String> convert(List<Metric> metrics) { |
There was a problem hiding this comment.
The convert logic is too complex, we need to find a way simplify it.
There was a problem hiding this comment.
yes because current metrics such as QueueMetrics and TPS etc is information metrics type , will improve later
There was a problem hiding this comment.
i hate current mechanism
everything is string.
i think our metrics result should be a model......
maybe we need to refactor metrics after this PR at once.
| return formattedMetrics; | ||
| } | ||
|
|
||
| class SeparatedOutputData { |
| super.subAppend(event); | ||
| } | ||
|
|
||
| public void setMaxBackupIndex(int maxBackups) { |
There was a problem hiding this comment.
why need to add this code?
base class already public it.
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.servicecomb</groupId> |
There was a problem hiding this comment.
java-chassis-core already depend on swagger-invocation-core
| public static final String METRICS_POLL_TIME = "servicecomb.metrics.polltime"; | ||
| public static final String METRICS_FILE_ENABLED = "servicecomb.metrics.file.enabled"; | ||
|
|
||
| private final int metricPoll; |
There was a problem hiding this comment.
formatted , but I only change one setting "blank lines around fields" to 0,not 1,I think is ok
There was a problem hiding this comment.
Fiexed,use current format style (set "blank lines around fields" to 1)
|
|
||
| //manage and init ServoObservers | ||
| @Component | ||
| public class MetricsServoObserverManager { |
There was a problem hiding this comment.
did not see what's the purpose of the manager.
it seems special for file?
There was a problem hiding this comment.
It use for init Servo PollScheduler,PollRunner and Observers
There was a problem hiding this comment.
change the class name to MetricsObserverInitializer and move it into io.servicecomb.foundation.metrics.output.servo package
|
|
||
| tpsAndLatencyMap.put("TPS Instance_Level", String.valueOf(insTotalTps)); | ||
| tpsAndLatencyMap.put("Latency Instance_Level", String.valueOf(instanceLatency)); | ||
| tpsAndLatencyMap.put("tps", String.valueOf(insTotalTps)); |
There was a problem hiding this comment.
of course，transaction per seconds
There was a problem hiding this comment.
confirmed , request may very low , current use double is better
| tpsAndLatencyMap.put("TPS Instance_Level", String.valueOf(insTotalTps)); | ||
| tpsAndLatencyMap.put("Latency Instance_Level", String.valueOf(instanceLatency)); | ||
| tpsAndLatencyMap.put("tps", String.valueOf(insTotalTps)); | ||
| tpsAndLatencyMap.put("latency", String.valueOf(instanceLatency)); |
There was a problem hiding this comment.
what's the length of latency string?
There was a problem hiding this comment.
add "servicecomb.metrics.round_places“ setting set round places ,default is 1
|
|
||
| public Microservice load() { | ||
| try { | ||
| //TODO: has any better way get appId and microserviceName ? new MicroserviceDefinition may heavy cost |
There was a problem hiding this comment.
appId and microserviceName support placeholder
but this code can not support it.
There was a problem hiding this comment.
sorry placeholder is what ? i don't see other process in RegistryUtils...
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(SimpleMetricsContentFormatter.class); | ||
| private final String applicationName; | ||
| private final ObjectMapper mapper = new ObjectMapper(); |
|
i don't think it's OK |
|
foundation <- common <- service-registry <- core <-(provider/handler/transport) but now, foundation depend on core...... |
|
For the dependency issue, foundation <- common <- service-registry <- core <-(provider/handler/transport) |
|
resolved dependency problem of foundation-metrics , current is |
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
…put only write file Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
…tion test (pojo) Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
…serviceName join string (metrics file name) Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
…n-metrics not dependency any other modules Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
zhengyangyong
force-pushed
the
JAV-526
branch
from
41d7591 to
230bea7
Compare
|
pull latest master from servicecomb and rebase on it |
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>
|
Please remember to update the document of how to use it? |
4 participants
https://servicecomb.atlassian.net/browse/JAV-526