Add pilot-agent support for configuring envoy to use the metrics service#11897
Add pilot-agent support for configuring envoy to use the metrics service#11897rshriram merged 1 commit intoistio:release-1.1from
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joeyb If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @joeyb. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
|
you need to update the helm templates for sidecar injection |
|
@rshriram - I added the helm template updates to the TODO list in the description. Btw, it's expected that the build for this would currently fail since that new ProxyConfig field doesn't actually exist yet. |
|
yes.. do it all in same PR |
|
@rshriram - I fleshed out the PR. I think this covers all the needed changes to propagate the new config. For right now, I just manually copied the api changes over. Once they actually get merged, I can properly update that vendored dep. |
There was a problem hiding this comment.
Could you combine this and the envoyStatsD into one single struct like this:
# These are statistics corresponding to the inner workings of the envoy proxies in the data plane.
# They contain detailed stat information emitted by filters in envoy as well as other subsystems in Envoy,
# gathered across multiple requests and connections. This should not be confused with Istio telemetry
# service that generates stats (e.g., response codes, latencies, etc.) per request or connection.
# Note that in addition to the following two sinks where Envoy pushes stats, prometheus style metrics can
# be scraped from the envoys using the admin port 127.0.0.1:15000, or via 127.0.0.1:15090/stats/prometheus
# Disabled by default
envoyInternalStats:
statsdSink
enabled: ..
...
grpcStatsReceiverSink:
enabled: ...
and fix up the existing statsD references as well?
The reason we need to do this is because the term metrics service is utterly generic, and ends up conflating with Istio metrics/telemetry. this will cause user confusion when they want to enable/disable pieces of Istio.
On a similar note, could you please rename the API field in istio/api to envoy_internal_stats_grpc_sink ?
cc @douglas-reid - this sound good?
There was a problem hiding this comment.
@rshriram yes. this sounds reasonable to me. thanks for taking on this bit of work.
There was a problem hiding this comment.
@rshriram - The reasoning behind that merger with the statsd config makes sense, but that would also be a breaking change for helm template users that modify those values. Is that an issue? I'm fine with making the change, just want to make sure we're not breaking any expected backwards compatibility guarantees.
There was a problem hiding this comment.
hmm true.. in that case lets keep them as two separate pieces but add the big blob on top.and probably name the metrics service thing as envoyInternalStatsGrpcReceiver for the lack of a more uglier lengthier name..
There was a problem hiding this comment.
dont forget to update the comment above
9375ff4 to
ed90243
Compare
|
@rshriram - All the prep work involving istio/api should be done now and the vendored dep for it has been fully updated, so this should be ready for full review now. |
|
please update that comment.. otherwise LGTM |
|
@rshriram - I added some details to that comment in a previous commit, but I just pushed another commit that expands on it a bit more. |
|
rebase please. I think the API update went in, in another PR by the bot. |
b4a350b to
471e647
Compare
|
@rshriram - The rebase got pretty messy, so I ended up squashing everything down to a single commit while cleaning everything up. Should be back in a good state now and ready to merge pending CI looking good. |
|
/test istio-pilot-e2e-envoyv2-v1alpha3 |
|
@rshriram - afaict, those tests are all just broken. scanning through other open PRs, it looks like they are failing for everyone. |
471e647 to
7942165
Compare
|
@joeyb: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@rshriram - can you take another look at this? I rebased against the latest from |
This PR is currently a WIP and dependent on istio/api#803. Similar to the statsd support, this extends the proxy config to support configuration of the envoy metrics service stats sink.
Remaining work:
bootstrap_config_test.gothat exercises the new config.