Support custom metric conversions in OTLP statSink #40854
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
TAOXUY
force-pushed
the
otlpMetricAggregation
branch
from
f9f89ae to
1805eb8
Compare
api/envoy/extensions/stat_sinks/open_telemetry/v3/open_telemetry.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
| return std::make_unique<OpenTelemetryGrpcSink>(otlp_metrics_flusher, grpc_metrics_exporter); | ||
| return std::make_unique<OpenTelemetryGrpcSink>( | ||
| otlp_metrics_flusher, grpc_metrics_exporter, | ||
| server.timeSource().systemTime().time_since_epoch().count()); |
There was a problem hiding this comment.
This looks like another change?
There was a problem hiding this comment.
This is the object construction time which will be used as the last flush time for this first flush.
| envoy::extensions::stat_sinks::open_telemetry::v3::SinkConfig::ConversionAction> { | ||
| public: | ||
| explicit OnMatchAction( | ||
| envoy::extensions::stat_sinks::open_telemetry::v3::SinkConfig::ConversionAction config) |
There was a problem hiding this comment.
Maybe pass by reference and return a reference?
There was a problem hiding this comment.
The caller create a temporary Action object so we have to do a copy.
I add a TODO to check if we can change that to a move operation.
https://screenshot.googleplex.com/7afqYYXURaDf4Lc
api/envoy/extensions/stat_sinks/open_telemetry/v3/open_telemetry.proto
Outdated
Show resolved
Hide resolved
source/extensions/stat_sinks/open_telemetry/open_telemetry_impl.cc
Outdated
Show resolved
Hide resolved
| @@ -107,7 +107,7 @@ class OpenTelemetryGrpcIntegrationTest : public Grpc::GrpcClientIntegrationParam | |||
| for (const opentelemetry::proto::metrics::v1::Metric& metric : metrics) { | |||
| if (metric.name() == getFullStatName("cluster.membership_change") && metric.has_sum()) { | |||
| known_counter_exists = true; | |||
| EXPECT_EQ(1, metric.sum().data_points().size()); | |||
| EXPECT_EQ(2, metric.sum().data_points().size()); | |||
There was a problem hiding this comment.
It is because we have aggregation logic so the metrics with the same name are aggregated now
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
api/envoy/extensions/matching/common_inputs/stats/v3/stats.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
Signed-off-by: Xuyang Tao <[email protected]>
|
/lgtm api |
|
@ohadvano Looking for your feedback as you are an owner for the extension. I can give a general review. |
See the proto file for the detailed feature description.