Conversation
Co-authored-by: Jose Nino <jnino@lyft.com> Signed-off-by: Jingwei Hao <jingwei.hao@gmail.com>
junr03
left a comment
junr03
left a comment
There was a problem hiding this comment.
great progress! First pass of comments.
| #include "extensions/filters/http/dynamic_forward_proxy/config.h" | ||
| #include "extensions/filters/http/router/config.h" | ||
| #include "extensions/filters/network/http_connection_manager/config.h" | ||
| #include "extensions/stat_sinks/metrics_service/config.h" |
There was a problem hiding this comment.
I would keep both around per my thought that it would be good, for a period of time, to emit metrics to both sinks and compare.
Additionally, we have not done the server work required to receive metrics from this sink, so we still need to emit using the old sink.
| "envoy.filters.http.router": "//source/extensions/filters/http/router:config", | ||
| "envoy.filters.network.http_connection_manager": "//source/extensions/filters/network/http_connection_manager:config", | ||
| "envoy.stat_sinks.metrics_service": "//source/extensions/stat_sinks/metrics_service:config", | ||
| "envoymobile.stat_sinks.metrics_service": "@envoy_mobile//library/common/extensions/stat_sinks/metrics_service:config", |
There was a problem hiding this comment.
| "envoymobile.stat_sinks.metrics_service": "@envoy_mobile//library/common/extensions/stat_sinks/metrics_service:config", | |
| "envoy.stat_sinks.metrics_service": "@envoy_mobile//library/common/extensions/stat_sinks/metrics_service:config", |
There was a problem hiding this comment.
We have kept the envoy namespacefor the other extensions. In this case we changed toenvoymobile` and kept the rest of the name the same.
@goaway do you have an opinion about changing the namespace to envoymobile versus keeping it in envoy and changing the nested types (which is what was done in the actual c++ classes?
| "envoy.transport_sockets.tls": "//source/extensions/transport_sockets/tls:config", | ||
| } | ||
| WINDOWS_EXTENSIONS = {} | ||
| WINDOWS_EXTENSIONS = {} No newline at end of file |
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "metrics_service_with_response", |
There was a problem hiding this comment.
| name = "metrics_service_with_response", | |
| name = "metrics_service", |
There was a problem hiding this comment.
I think the with_response suffix is a little clunky. I'd rather keep the name shorter and write a good comment in the header file that explains what is different from the streamer here vs in envoy.
There was a problem hiding this comment.
It's also inconsistent with the class name chosen below EnvoyMobileMetricsService.
| @@ -0,0 +1,34 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package envoymobile.extensions.StatSinks.MetricsService; | |||
There was a problem hiding this comment.
| package envoymobile.extensions.StatSinks.MetricsService; | |
| package envoymobile.extensions.stat_sinks.metrics_service; |
There was a problem hiding this comment.
Note that this means we will have to change this type in other places (including the service_method).
|
|
||
| void send(MetricsService::MetricsPtr&& metrics) override; | ||
|
|
||
| void onReceiveMessage( |
There was a problem hiding this comment.
move the method definition (body) to the cc file. This method will eventually be larger.
| } // namespace EnvoyMobileMetricsService | ||
| } // namespace StatSinks | ||
| } // namespace Extensions | ||
| } // namespace Envoy No newline at end of file |
There was a problem hiding this comment.
newline. You can run the formaters locallly with ./tools/check_format.sh fix and also pre-commit run --all-files
| // Send an empty response and end the stream. This should never happen but make sure nothing | ||
| // breaks and we make a new stream on a follow up request. | ||
| metrics_service_request_->startGrpcStream(); | ||
| envoymobile::extensions::StatSinks::MetricsService::EnvoyMobileStreamMetricsResponse response_msg; |
There was a problem hiding this comment.
We should set a batch id and verify it.
| InSequence s; | ||
|
|
||
| // Start a stream and send first message. | ||
|
|
| std::make_unique<Envoy::Protobuf::RepeatedPtrField<io::prometheus::client::MetricFamily>>(); | ||
| streamer_->send(std::move(metrics)); | ||
| } | ||
|
|
There was a problem hiding this comment.
There should be a test for receiving a response message.
|
opened #1183 instead |
2 participants
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.
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 jingwei.hao@gmail.com