stats: adds metrics sink with ack#1183
Merged
junr03 merged 4 commits intoenvoyproxy:mainfrom Feb 3, 2021
jingwei99:grpc_streamer
Merged
stats: adds metrics sink with ack#1183junr03 merged 4 commits intoenvoyproxy:mainfrom jingwei99:grpc_streamer
junr03 merged 4 commits intoenvoyproxy:mainfrom
jingwei99:grpc_streamer
Conversation
This was referenced Jan 4, 2021
junr03
reviewed
Jan 22, 2021
Member
junr03
left a comment
There was a problem hiding this comment.
mostly lgtm! Awesome. A small comment
| // A list of metric entries | ||
| repeated io.prometheus.client.MetricFamily envoy_metrics = 2; | ||
|
|
||
| string batch_id = 3; |
Member
There was a problem hiding this comment.
nit: I would make this field 2.
Also lets document what this field is for.
Contributor
Author
There was a problem hiding this comment.
you mean
string batch_id = 2;
repeated io.prometheus.client.MetricFamily envoy_metrics = 3;
?
👌 on documentation
| grpc_service: | ||
| envoy_grpc: | ||
| cluster_name: stats | ||
| - name: envoy.stat_sinks.metrics_service.mobile |
Member
There was a problem hiding this comment.
I am going to put up a PR today to reduce some of the stats we are emitting (and no longer need), as this config effectively doubles the amount of stats emitted
junr03
previously approved these changes
Feb 1, 2021
| } | ||
|
|
||
| message EnvoyMobileStreamMetricsResponse { | ||
| string batch_id = 1; |
| // only be sent in the first message on the stream. | ||
| Identifier identifier = 1; | ||
|
|
||
| // Identifier for the current envoy_metrics batch |
Member
There was a problem hiding this comment.
Suggested change
| // Identifier for the current envoy_metrics batch | |
| // Identifier for the current envoy_metrics batch. |
Same below.
added 4 commits
February 2, 2021 12:51
Co-authored-by: Jose Nino jnino@lyft.com Signed-off-by: Jingwei Hao <jingweih@lyft.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description: Replace the metrics sink from the envoy one to a custom one. The custom metrics sink comes with ack function on the grpc stream. This change is based on upstream envoy change: envoyproxy/envoy#13919
Risk Level: High
Testing: Local and ci unit tests and integration tests
Co-authored-by: Jose Nino jnino@lyft.com
Signed-off-by: Jingwei Hao jingweih@lyft.com