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 listeners metrics #3922

Merged
merged 3 commits into from
Jun 19, 2019
Merged

Add listeners metrics #3922

merged 3 commits into from
Jun 19, 2019

Conversation

coignetp
Copy link
Contributor

What does this PR do?

Add more metrics

Motivation

Customer request

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@coignetp coignetp requested a review from a team as a code owner June 14, 2019 16:23
@therve therve changed the title Add listerners metrics Add listeners metrics Jun 14, 2019
@FlorianVeaux
Copy link
Member

Looks good, one question though. The customer request was also to add metrics for cipher:

$ curl -s 127.0.0.1:8002/stats | grep listener.0.0.0.0_8443.ssl.ciphers
listener.0.0.0.0_8443.ssl.ciphers.AEAD-AES128-GCM-SHA256: 4
listener.0.0.0.0_8443.ssl.ciphers.AEAD-AES256-GCM-SHA384: 3
listener.0.0.0.0_8443.ssl.ciphers.ECDHE-RSA-AES128-GCM-SHA256: 2495
listener.0.0.0.0_8443.ssl.ciphers.ECDHE-RSA-AES128-SHA: 10471
listener.0.0.0.0_8443.ssl.ciphers.ECDHE-RSA-CHACHA20-POLY1305: 142

Any chance they could be added too?

@codecov
Copy link

codecov bot commented Jun 17, 2019

Codecov Report

Merging #3922 into master will increase coverage by 7.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3922      +/-   ##
==========================================
+ Coverage   86.87%   93.93%   +7.06%     
==========================================
  Files         705       13     -692     
  Lines       36265      610   -35655     
  Branches     4336       98    -4238     
==========================================
- Hits        31505      573   -30932     
+ Misses       3557       24    -3533     
+ Partials     1203       13    -1190

1 similar comment
@codecov
Copy link

codecov bot commented Jun 17, 2019

Codecov Report

Merging #3922 into master will increase coverage by 7.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3922      +/-   ##
==========================================
+ Coverage   86.87%   93.93%   +7.06%     
==========================================
  Files         705       13     -692     
  Lines       36265      610   -35655     
  Branches     4336       98    -4238     
==========================================
- Hits        31505      573   -30932     
+ Misses       3557       24    -3533     
+ Partials     1203       13    -1190

@coignetp
Copy link
Contributor Author

Fixed the customer request by adding ciphers. The issue was probably a typo here

. I leave it there (and in the metadata
envoy.listener.ssl.cipher,count,,connection,,Total TLS connections that used cipher tag,0,envoy,
) not to have breaking changes.

@FlorianVeaux
Copy link
Member

👍 I think we should be fine to remove it, as this metric is never emitted right? I've checked and there isn't any customer monitor relying on this metric.

@hithwen
Copy link
Contributor

hithwen commented Jun 18, 2019

@FlorianVeaux thanks for checking!

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

🔥

@ofek
Copy link
Contributor

ofek commented Jun 19, 2019

@coignetp @FlorianVeaux No typo, when I did this it was different https://www.envoyproxy.io/docs/envoy/v1.5.0/configuration/listeners/stats

@coignetp coignetp merged commit b22a7cf into master Jun 19, 2019
@coignetp coignetp deleted the paul/add-envoy-metrics branch June 19, 2019 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants