-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(observability): count byte_size after transforming event #17941
Changes from all commits
38c2a15
7f13f70
acf5cd3
6a808c9
64b7d8c
c825ccd
1dc1637
553223d
cd7a4f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
//! Constants for commonly used semantic meanings. | ||
|
||
/// The service typically represents the application that generated the event. | ||
pub const SERVICE: &str = "service"; | ||
|
||
/// The main text message of the event. | ||
pub const MESSAGE: &str = "message"; | ||
|
||
/// The main timestamp of the event. | ||
pub const TIMESTAMP: &str = "timestamp"; | ||
|
||
/// The hostname of the machine where the event was generated. | ||
pub const HOST: &str = "host"; | ||
|
||
pub const SOURCE: &str = "source"; | ||
pub const SEVERITY: &str = "severity"; | ||
pub const TRACE_ID: &str = "trace_id"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
mod definition; | ||
pub mod meaning; | ||
mod requirement; | ||
|
||
pub use definition::Definition; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ use crate::sinks::prelude::*; | |
use bytes::BytesMut; | ||
use std::io; | ||
use tokio_util::codec::Encoder as _; | ||
use vector_core::config::telemetry; | ||
|
||
#[derive(Clone, Debug)] | ||
pub(super) struct AmqpEncoder { | ||
|
@@ -11,9 +12,17 @@ pub(super) struct AmqpEncoder { | |
} | ||
|
||
impl encoding::Encoder<Event> for AmqpEncoder { | ||
fn encode_input(&self, mut input: Event, writer: &mut dyn io::Write) -> io::Result<usize> { | ||
fn encode_input( | ||
&self, | ||
mut input: Event, | ||
writer: &mut dyn io::Write, | ||
) -> io::Result<(usize, GroupedCountByteSize)> { | ||
let mut body = BytesMut::new(); | ||
self.transformer.transform(&mut input); | ||
|
||
let mut byte_size = telemetry().create_request_count_byte_size(); | ||
byte_size.add_event(&input, input.estimated_json_encoded_size_of()); | ||
|
||
let mut encoder = self.encoder.clone(); | ||
encoder | ||
.encode(input, &mut body) | ||
Comment on lines
+24
to
28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit outside the scope of the PR, but I'm curious why we need to estimate the JSON size right before we actually encode the event. Don't we have the actual size here that we could use directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was part of #17576. The intention was to use a measurement that could be taken anywhere in the processing pipeline (source, transforms, sinks) so that the values are comparable. |
||
|
@@ -22,6 +31,6 @@ impl encoding::Encoder<Event> for AmqpEncoder { | |
let body = body.freeze(); | ||
write_all(writer, 1, body.as_ref())?; | ||
|
||
Ok(body.len()) | ||
Ok((body.len(), byte_size)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for these two paths to get out of sync? E.g. if you drop a field with a given meaning and then subsequently write a new field and assign it that same meaning? Or is that not a problem because we're only using this in the
{only,except}_fields
path?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a very valid point. It's not a problem here because the dropping and retrieving the value all occur in the sink, so there is no opportunity for a new value to be added. However, there is a risk that a future change could stumble on this. I have added a comment to warn about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do end up going with some kind of change like #18028 then it would be nice to undo this just to get rid of the oddity in the API and potential for mismatch. I'm not sure if that's just something like a TODO on the field or an issue, but maybe worth noting so that this doesn't stick around in the event that it becomes unneeded.