feature: add kernel info to EngineVersion metric #2889
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #2889 +/- ##
==========================================
- Coverage 68.86% 68.86% -0.01%
==========================================
Files 291 291
Lines 18249 18249
==========================================
- Hits 12568 12567 -1
- Misses 4224 4230 +6
+ Partials 1457 1452 -5
|
ZYecho
reviewed
Jun 4, 2019
|
Could you please add test? There is an example case would be helpful. |
KevinBetterQ
changed the title
KevinBetterQ
force-pushed
the
2881-add-kernel-info
branch
from
600d701
to
938058f
Compare
KevinBetterQ
changed the title
CodeJuan
reviewed
Jun 4, 2019
test/api_system_metrics_test.go
Outdated
| func (suite *APIEngineVersionMetricsSuite) TestEngineVersionMetrics(c *check.C) { | ||
| key := "engine_daemon_engine_info" | ||
| hasMetric := HasMetric(c, key) | ||
| c.Assert(hasMetric, check.Equals, true) |
There was a problem hiding this comment.
I'm afraid hasMetric=true isn't good enough to check all label. How about to assert the value of label kernel?
There was a problem hiding this comment.
add matching label checks
CodeJuan
reviewed
Jun 4, 2019
apis/metrics/metrics.go
Outdated
| @@ -38,7 +38,7 @@ var ( | |||
| ImageActionsTimer = metrics.NewLabelTimer(subsystemPouch, "image_actions", "The number of seconds it takes to process each image action", "action") | |||
|
|
|||
| // EngineVersion records the version and commit information of the engine process. | |||
| EngineVersion = metrics.NewLabelGauge(subsystemPouch, "engine", "The version and commit information of the engine process", "commit") | |||
| EngineVersion = metrics.NewLabelGauge(subsystemPouch, "engine", "The version and commit information of the engine process", "gitCommit", "pouchVersion", "kernelVersion") | |||
There was a problem hiding this comment.
The name of labels is used by monitoring system, so it would be better to stay the same
There was a problem hiding this comment.
keep "commit", add "version" and "kernel"
KevinBetterQ
force-pushed
the
2881-add-kernel-info
branch
3 times, most recently
from
579a86e
to
6e4062f
Compare
|
KevinBetterQ
force-pushed
the
2881-add-kernel-info
branch
2 times, most recently
from
8a0c924
to
1e229b3
Compare
fuweid
reviewed
Jun 5, 2019
test/api_system_metrics_test.go
Outdated
|
|
||
| // SetUpTest does common setup in the beginning of each test. | ||
| func (suite *APIEngineVersionMetricsSuite) SetUpTest(c *check.C) { | ||
| SkipIfFalse(c, environment.IsLinux) |
There was a problem hiding this comment.
please remove the setuptest and teardownsuite because the setupsuite is good enough
There was a problem hiding this comment.
ok, already deleted
Signed-off-by: KevinBetterQ <1093850932@qq.com>
KevinBetterQ
force-pushed
the
2881-add-kernel-info
branch
from
1e229b3
to
4052bf8
Compare
fuweid
reviewed
Jun 5, 2019
|
|
||
| regularVersion := `^.*version="(.*?)".*$` | ||
| regular = regexp.MustCompile(regularVersion) | ||
| params = regular.FindStringSubmatch(versionMetrics) |
There was a problem hiding this comment.
maybe we should check the length here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ⅰ. Describe what this PR did
add kernel info to EngineVersion metric
Ⅱ. Does this pull request fix one issue?
fixes #2881
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
added
Ⅳ. Describe how to verify it
$ pouchd -l tcp://0.0.0.0:4243$ curl http://127.0.0.1:4243/metricsⅤ. Special notes for reviews