Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Armeria version metric #2179

Merged
merged 7 commits into from Oct 18, 2019
Merged

Conversation

@matsumana
Copy link
Member

matsumana commented Oct 10, 2019

Hello, I've added Armeria version metric.

Motivation:

  • Want to check Armeria version with Grafana
    FYI:
    In Prometheus best practices, this kind of metric is described as follows.
    https://prometheus.io/docs/practices/naming/

    foobar_build_info (for a pseudo-metric that provides metadata about the running binary)

    e.g.

    # HELP node_exporter_build_info A metric with a constant '1' value labeled by version, revision, branch, and goversion from which node_exporter was built.
    # TYPE node_exporter_build_info gauge
    node_exporter_build_info{branch="master",goversion="go1.7.5",revision="840ba5dcc71a084a3bc63cb6063003c1f94435a6",version="0.14.0"} 1
    

Modifications:

  • Add Armeria version metric

Result:

It's exported as follows

Labels:

  • commit
  • version
# HELP armeria_build_info A metric with a constant '1' value labeled by version and commit hash from which Armeria was built.
# TYPE armeria_build_info gauge
armeria_build_info{commit="74700d402178e00045b1ca23006f7f6131b2a92d",version="0.94.1-SNAPSHOT",} 1.0

example dashboard:

@matsumana matsumana requested review from ikhoon, minwoox and trustin as code owners Oct 10, 2019
@ikhoon ikhoon added the new-feature label Oct 10, 2019
@trustin

This comment has been minimized.

Copy link
Member

trustin commented Oct 10, 2019

That is awesome, @matsumana. I love it! What do you think about adding other labels such as repoStatus whose value is clean or dirty?

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #2179 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2179      +/-   ##
============================================
- Coverage     73.62%   73.61%   -0.02%     
+ Complexity     9581     9580       -1     
============================================
  Files           837      837              
  Lines         36883    36898      +15     
  Branches       4548     4548              
============================================
+ Hits          27155    27162       +7     
- Misses         7402     7412      +10     
+ Partials       2326     2324       -2
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/linecorp/armeria/server/Server.java 83.92% <100%> (+1%) 29 <2> (+2) ⬆️
.../linecorp/armeria/client/Http2ResponseDecoder.java 60.83% <0%> (-3.34%) 32% <0%> (-1%)
...necorp/armeria/server/GracefulShutdownSupport.java 97.43% <0%> (-2.57%) 4% <0%> (ø)
...com/linecorp/armeria/server/saml/SamlEndpoint.java 62.5% <0%> (-2.5%) 10% <0%> (-1%)
...rp/armeria/common/stream/DefaultStreamMessage.java 89.36% <0%> (-1.42%) 53% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84d3b36...a61269b. Read the comment docs.

Copy link
Collaborator

anuraaga left a comment

Cool feature

final MeterRegistry meterRegistry = server.config().meterRegistry();
assertThat(meterRegistry.find("armeria.build.info")
.gauge()
.value())

This comment has been minimized.

Copy link
@anuraaga

anuraaga Oct 11, 2019

Collaborator

How about also checking the three tags exist (though not checking their values which isn't constant)

@matsumana

This comment has been minimized.

Copy link
Member Author

matsumana commented Oct 11, 2019

What do you think about adding other labels such as repoStatus whose value is clean or dirty?

Sounds good.
Added!

# HELP armeria_build_info A metric with a constant '1' value labeled by version and commit hash from which Armeria was built.
# TYPE armeria_build_info gauge
armeria_build_info{commit="74700d402178e00045b1ca23006f7f6131b2a92d",repostatus="dirty",version="0.94.1-SNAPSHOT",} 1.0

I added repostatus, not repoStatus.
I followed node_exporter's label naming.
e.g.
node_filesystem_free{device="/dev/sda2",fstype="ext4",mountpoint="/"} 8.4968587264e+10

@anuraaga

This comment has been minimized.

assertThat(gauge).isNotNull()

Copy link
Collaborator

anuraaga left a comment

Thanks!

Copy link
Member

minwoox left a comment

Thanks!

@ikhoon
ikhoon approved these changes Oct 14, 2019
Copy link
Contributor

ikhoon left a comment

👍 Thanks! @matsumana

@ikhoon ikhoon added this to the 0.95.0 milestone Oct 14, 2019
Copy link
Member

trustin left a comment

Just two nits. Let me push a change directly to your branch and merge. Thank you!

trustin added 2 commits Oct 17, 2019
@trustin

This comment has been minimized.

Copy link
Member

trustin commented Oct 17, 2019

I added repostatus, not repoStatus.
I followed node_exporter's label naming.

Oh, just found this comment. Armeria currently is using camel case convention for label names, e.g. hostnamePattern, so I changed it to repoStatus. Do you think we should make them all lower-cased?

@matsumana

This comment has been minimized.

Copy link
Member Author

matsumana commented Oct 17, 2019

Oh, just found this comment. Armeria currently is using camel case convention for label names, e.g. hostnamePattern, so I changed it to repoStatus. Do you think we should make them all lower-cased?

It was my misunderstanding.
I remembered that you guys were discussing about metrics naming conventions in a issue/PR before, so I misunderstood that you are going to migrate from camelCase.

Thank you for changing the label.
Could you merge?

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Oct 17, 2019

Ah, it was about meter names rather than label names :-) Will merge soon!

@trustin trustin merged commit 29e55e7 into line:master Oct 18, 2019
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 73.62%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +26.37% compared to 84d3b36
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@trustin

This comment has been minimized.

Copy link
Member

trustin commented Oct 18, 2019

Much appreciated, @matsumana ! 👍

@matsumana

This comment has been minimized.

Copy link
Member Author

matsumana commented Oct 18, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.