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.

Sign up for GitHub

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

Jump to bottom

*: Change gRPC proxy to expose endpoint /metrics #10618

Merged
merged 2 commits into from
Apr 10, 2019
Merged

*: Change gRPC proxy to expose endpoint /metrics #10618

merged 2 commits into from
Apr 10, 2019

Conversation

hexfusion
Copy link
Contributor

@hexfusion hexfusion commented Apr 5, 2019 •
edited
Loading

Currently, grpc-proxy exposes only metrics for proxy itself and does not allow the client to consume endpoint /metrics. The /health endpoint forwards a request against endpoint and so should /metrics. This PR resolves an issue where metrics returned by /metrics were the metrics of the proxy itself instead of the etcd member server.

Fixes: #10611

@hexfusion hexfusion requested a review from gyuho April 5, 2019 19:26
@hexfusion hexfusion force-pushed the proxy_metrics branch 4 times, most recently from 4e4ae90 to ddde6ef Compare April 6, 2019 17:13
@codecov-io
Copy link

codecov-io commented Apr 6, 2019 •
edited
Loading

Codecov Report

Merging #10618 into master will decrease coverage by 0.11%.
The diff coverage is 32.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10618      +/-   ##
==========================================
- Coverage   71.76%   71.65%   -0.12%     
==========================================
  Files         393      393              
  Lines       36571    36632      +61     
==========================================
+ Hits        26247    26249       +2     
- Misses       8495     8555      +60     
+ Partials     1829     1828       -1
Impacted Files Coverage Δ
proxy/grpcproxy/metrics.go 25% <10%> (-75%) ⬇️
etcdserver/api/etcdhttp/metrics.go 85% <100%> (ø) ⬆️
etcdmain/grpc_proxy.go 60.53% <48.57%> (-2.08%) ⬇️
pkg/adt/interval_tree.go 84.98% <0%> (-6.31%) ⬇️
clientv3/namespace/watch.go 87.87% <0%> (-6.07%) ⬇️
etcdctl/ctlv3/command/lease_command.go 65.34% <0%> (-5.95%) ⬇️
etcdserver/api/v3rpc/watch.go 80.06% <0%> (-4.58%) ⬇️
pkg/transport/listener.go 54.5% <0%> (-3.8%) ⬇️
clientv3/concurrency/election.go 79.68% <0%> (-2.35%) ⬇️
etcdserver/server.go 74.53% <0%> (-0.44%) ⬇️
... and 19 more

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 a621d80...35a58c4. Read the comment docs.

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 8, 2019 •
edited
Loading

Original implementation by #8242 was meant to add metrics to proxy not expose proxy metrics. So this can be backported to release-3.3 as bug fix.

/cc @gyuho

gyuho
gyuho reviewed Apr 8, 2019
View reviewed changes
proxy/grpcproxy/metrics.go Outdated
pathMetrics := etcdhttp.PathMetrics
target := fmt.Sprintf("%s%s", eps[0], pathMetrics)
if !strings.HasPrefix(target, "http") {
target = fmt.Sprintf("%s%s", "http://", target)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't handle https here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reads if not if !strings.HasPrefix(target, "http")

if scheme = https target does not change as http matches https as well. If no scheme we assume http.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we are setting the scheme http://?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://play.golang.org/p/PENCHQL99GQ

Copy link
Contributor

Choose a reason for hiding this comment

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

What if "member:2379" were for TLS endpoints? I remember scheme was not required for etcd gRPC endpoints.

Could you check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Get fails without scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you check?

sounds good

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated is that what you were thinking?

@hexfusion hexfusion force-pushed the proxy_metrics branch 2 times, most recently from 9545f68 to da742ff Compare April 8, 2019 19:56
@gyuho
Copy link
Contributor

gyuho commented Apr 8, 2019

@hexfusion Can we update the doc and changelog?

@hexfusion hexfusion force-pushed the proxy_metrics branch 5 times, most recently from 7c8cebc to d5a31db Compare April 9, 2019 16:29
@hexfusion
Copy link
Contributor Author

@gyuho updated.

gyuho
gyuho reviewed Apr 9, 2019
View reviewed changes
CHANGELOG-3.4.md Outdated Show resolved Hide resolved
@hexfusion hexfusion changed the title *: allow gRPC proxy to expose to endpoint /metrics *: Change gRPC proxy to expose endpoint /metrics Apr 9, 2019
@hexfusion hexfusion force-pushed the proxy_metrics branch 3 times, most recently from e73f7ae to 2bc147f Compare April 10, 2019 13:22
@hexfusion
Copy link
Contributor Author

Semaphore failure TestCtlV3AuthFromKeyPerm nonrelated to these changes.

@hexfusion
Copy link
Contributor Author

@gyuho ptal

gyuho
gyuho reviewed Apr 10, 2019
View reviewed changes
CHANGELOG-3.4.md Outdated Show resolved Hide resolved
gyuho
gyuho reviewed Apr 10, 2019
View reviewed changes
CHANGELOG-3.4.md Outdated Show resolved Hide resolved
@hexfusion
*: Change gRPC proxy to expose etcd server endpoint /metrics
428ef62 
This PR resolves an issue where the `/metrics` endpoints exposed by the proxy were not returning metrics of the etcd members servers but of the proxy itself.

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion
CHANGELOG: add "Change gRPC proxy to expose etcd server endpoint /met…
ebafb2a 
…rics" PR

Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion
Copy link
Contributor Author

Updated and per @gyuho #10618 (comment) I am merging with greens.

@hexfusion hexfusion merged commit 9d62477 into etcd-io:master Apr 10, 2019
@hexfusion
Copy link
Contributor Author

@gyuho thank you for the review

@hexfusion hexfusion deleted the proxy_metrics branch April 10, 2019 20:09
This was referenced Apr 11, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gyuho gyuho gyuho left review comments

Assignees
No one assigned
Labels
backport/v3.3
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

grpc-proxy: listenAddr/metrics exposes metrics of proxy -> endpoint not etcd server endpoint.
3 participants
@hexfusion @codecov-io @gyuho