Skip to content

Add Metrics Service based on Micrometer.#1303

Closed
JulianFeinauer wants to merge 20 commits intomasterfrom
feature/improve-monitoring-metrics
Closed

Add Metrics Service based on Micrometer.#1303
JulianFeinauer wants to merge 20 commits intomasterfrom
feature/improve-monitoring-metrics

Conversation

@JulianFeinauer
Copy link
Copy Markdown
Contributor

@JulianFeinauer JulianFeinauer commented Jun 1, 2020

  • Prometheus Endpoint is exposed based on Micrometer Prometheus.
  • Add IoTDBRegistry which stores all Metrics as IoTDB timeseries under root._metrics
  • Added Monitoring at several points in the programm
  • Added IService Implementation for new Service

This is not final but rather a Work-In-Process where I would like to get Feedback regarding the Architecture.

@JulianFeinauer JulianFeinauer requested a review from jixuan1989 June 1, 2020 19:37
@JulianFeinauer JulianFeinauer marked this pull request as draft June 1, 2020 20:01
@jixuan1989
Copy link
Copy Markdown
Member

jixuan1989 commented Jun 9, 2020

some suggestions:

  1. use latency rather than duration;

  2. use InsertPlan to write data rather than use SQL;

  3. I think it is better to keep the server module as lightweight as possible;

  • add a switch to enable/disable collect metrics (please read the 4th suggestion. If the performance impaction can be ignored, we do not need this switch);
  • add a switch to enable/disable writing metrics into IoTDB
  • move the HTTP service and Prometheus into a new module, e.g., Prometheus-registry. Then the dependency of micrometer-registry-prometheus can be moved out of the server module.
    (1) To implement it, you may have to extract some implement in MicrometerServerService.java as an interface, and just retain the IoTDBRegistry as the default implementation;
    (2) In prometheus-registry module, you can add the dependency and reimplement the interface.
    (3) suppose we have a new module prometheus-registry, do not let the server module depends on it. If so, we may need using the Java reflect strategy to load the new implementation in (2).
  1. better to attach a report that the performance impaction after adding this feature

jixuan1989
jixuan1989 reviewed Jun 10, 2020
View reviewed changes
micrometer-registry-iotdb/src/main/java/org/apache/iotdb/micrometer/IoTDBRegistryConfig.java Outdated
@@ -0,0 +1,61 @@
/*
* 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.
Copy link
Copy Markdown
Member

@jixuan1989 jixuan1989 Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incorrect apache header format

jixuan1989
jixuan1989 reviewed Jun 10, 2020
View reviewed changes
server/src/main/java/org/apache/iotdb/db/service/MetricsService.java Outdated
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
* 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.
Copy link
Copy Markdown
Member

@jixuan1989 jixuan1989 Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format

@JulianFeinauer JulianFeinauer marked this pull request as ready for review June 11, 2020 07:12
@JulianFeinauer
Add Metrics Service based on Micrometer.
c4408c4 
* Prometheus Endpoint is exposed based on Micrometer Prometheus.
* Add IoTDBRegistry which stores all Metrics as IoTDB timeseries under root._metrics
* Added Monitoring at several points in the programm
* Added IService Implementation for new Service
@JulianFeinauer
Fixed issues with Sonar.
3269369
@JulianFeinauer
IOTDB-23 several improvements and fixes failing tests.
7e71c4e
@JulianFeinauer
IOTDB-23 Add Support for more Metrics in IoTDB.
05ab9c8
@JulianFeinauer
IOTDB-23, IOTDB-756, IOTDB-757 Replaced current Metrics implementatio…
9b2bb95 
…n with a new Implementation based on Micrometer and Bindings.

- Added Support for Prometheus Endpoint
- Added Support for IoTDB Based Consumer based on Session API
- Added default Bindings for JVM, Host, ...
- Added some Metrics at crucial places
- Slight refactoring, moved ITs from Sessions API to Server to avoid circular dependencies with additional Modules
@JulianFeinauer
IOTDB-23, IOTDB-756, IOTDB-757 Added fine-grained Configuration for M…
82b3f88 
…etrics Service.
@JulianFeinauer
IOTDB-23, IOTDB-756, IOTDB-757 Changed duration -> latency as suggest…
e383f9e 
…ed during Code Review.
@JulianFeinauer
IOTDB-23, IOTDB-756, IOTDB-757 Fixed Compilation error.
2b95f8d
@JulianFeinauer
Fixed issue.
85f017c
@JulianFeinauer
Fixed Sonar issues and other problems.
0ed6a42
@JulianFeinauer
Some fixes after rebase.
8b490ef
@JulianFeinauer JulianFeinauer force-pushed the feature/improve-monitoring-metrics branch from 37e8d72 to 8b490ef Compare June 11, 2020 09:11
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jun 26, 2020

SonarCloud Quality Gate failed.

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 42 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@jixuan1989 jixuan1989 mentioned this pull request Nov 8, 2020
Merged
@JulianFeinauer JulianFeinauer mentioned this pull request Apr 4, 2021
Closed
11 tasks
@liangyuanpeng
Copy link
Copy Markdown

liangyuanpeng commented Jun 5, 2021

So what problem have this PR now?

@jixuan1989
Copy link
Copy Markdown
Member

jixuan1989 commented Nov 9, 2021

Hi Julian, please see https://cwiki.apache.org/confluence/display/IOTDB/Monitor+Module
and
#4194

@SteveYurongSu
Copy link
Copy Markdown
Member

SteveYurongSu commented Apr 2, 2022

The new metrics framework has been accepted by the community, and the implementation of this PR has been referenced.

Thanks, Julian! @JulianFeinauer

I will close this PR.

@SteveYurongSu SteveYurongSu deleted the feature/improve-monitoring-metrics branch April 2, 2022 09:09
@SteveYurongSu SteveYurongSu restored the feature/improve-monitoring-metrics branch April 2, 2022 09:09
@JulianFeinauer
Copy link
Copy Markdown
Contributor Author

JulianFeinauer commented Apr 4, 2022

Thanks for the feedback @SteveYurongSu and congrats to the effort!

@qiaojialin qiaojialin deleted the feature/improve-monitoring-metrics branch April 26, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jixuan1989 jixuan1989 jixuan1989 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JulianFeinauer @jixuan1989 @liangyuanpeng @SteveYurongSu