Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
2031a4b to
0980921
Compare
blt
force-pushed
the
blt/add_otlpmetric_variant_body_to_http_generator
branch
from
24189a2 to
782b23f
Compare
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
0980921 to
37ff197
Compare
blt
force-pushed
the
blt/add_otlpmetric_variant_body_to_http_generator
branch
2 times, most recently
from
ab8327a to
698d55b
Compare
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
37ff197 to
4813280
Compare
blt
changed the base branch from
blt/add_otlpmetric_variant_body_to_http_generator
to
graphite-base/1365
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
4813280 to
55cba53
Compare
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
55cba53 to
8d31a7d
Compare
| let request = request.into_inner(); | ||
| let size = request.encoded_len(); | ||
|
|
||
| counter!("bytes_received", &self.labels).increment(size as u64); |
There was a problem hiding this comment.
This blackhole produces multiple bytes_received series and will be incompatible with regression detector ingress analysis. No change required, just something that might be surprising & additional motivation to do aggregation in analysis.
There was a problem hiding this comment.
I hadn't remember this but looking at this now, yeah, I feel like it strongly supports aggregation in analysis. Else we end up with sort of a goofy setup here.
lading/src/blackhole/otlp/http.rs
Outdated
| req: Request<hyper::body::Incoming>, | ||
| ) -> Result<Response<BoxBody<Bytes, hyper::Error>>, hyper::Error> { | ||
| counter!("requests_received", &self.labels).increment(1); | ||
|
|
There was a problem hiding this comment.
Two thoughts on Content-Type. Should this apply a label by incoming content type?
Is this compatible with JSON-encoded protobufs? Naively I see serde support in the otel-proto crate, but I don't expect prost's decode to attempt JSON deserialization. I'm not familiar with Rust's JSON proto situation, but I think there might be a piece missing here.
There was a problem hiding this comment.
Two thoughts on Content-Type. Should this apply a label by incoming content type?
I don't see why not.
Is this compatible with JSON-encoded protobufs? Naively I see serde support in the otel-proto crate, but I don't expect prost's decode to attempt JSON deserialization. I'm not familiar with Rust's JSON proto situation, but I think there might be a piece missing here.
Let me look. Now that you ask I assumed that the decode would happen transparently but that's not a given.
There was a problem hiding this comment.
Yep, that was entirely broken. Good catch. Handled in 2e6decc.
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
dc1df06 to
ea0a742
Compare
truthbk
left a comment
truthbk
left a comment
There was a problem hiding this comment.
Won't comment on lading, or SMP specifics, but this blackhole implementation makes sense to me (slight concern on the aggregation in analysis and the current "incompatibility" with the regression detector analysis, and what its implications are today to compare performance and make claims).
Yeah. Having slept on this, I'm going to remove the incompatibility and we'll lose the inability to make claims that one signal received more bytes than another or that one protocol received more than another but we won't goof analysis. |
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
2e6decc to
7235a60
Compare
The handling of Otlp is specialized enough that having a distinct blackhole for it makes good sense, especially because we want to be able to support gRPC request/response and could not with our existing blackhole. In a subsequent PR I will remove the OtlpMetrics support in the http blackhole. I believe I also understand how how to make an otlp_grpc generator. REF SMPTNG-659 Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
Signed-off-by: Brian L. Troutwine <brian.troutwine@datadoghq.com>
blt
force-pushed
the
blt/create_otlp_specific_blackhole
branch
from
7235a60 to
af52d5a
Compare
3 participants
What does this PR do?
The handling of Otlp is specialized enough that having a distinct
blackhole for it makes good sense, especially because we want to be
able to support gRPC request/response and could not with our existing
blackhole. In a subsequent PR I will remove the OtlpMetrics support
in the http blackhole. I believe I also understand how how to make
an otlp_grpc generator.
Motivation
REF SMPTNG-659