-
Notifications
You must be signed in to change notification settings - Fork 2k
chore(observability): consolidate EventCountTags with TaggedEventsSent
#17865
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
Changes from all commits
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 |
|---|---|---|
| @@ -1,44 +1,18 @@ | ||
| use std::collections::HashMap; | ||
| use std::ops::Add; | ||
| use std::{collections::HashMap, sync::Arc}; | ||
|
|
||
| use crate::{ | ||
| config::ComponentKey, | ||
| internal_event::{ | ||
| CountByteSize, InternalEventHandle, OptionalTag, RegisterTaggedInternalEvent, | ||
| RegisteredEventCache, | ||
| CountByteSize, InternalEventHandle, RegisterTaggedInternalEvent, RegisteredEventCache, | ||
| TaggedEventsSent, | ||
| }, | ||
| json_size::JsonSize, | ||
| }; | ||
|
|
||
| /// Tags that are used to group the events within a batch for emitting telemetry. | ||
| #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct EventCountTags { | ||
| pub source: OptionalTag<Arc<ComponentKey>>, | ||
| pub service: OptionalTag<String>, | ||
| } | ||
|
Comment on lines
-14
to
-18
Member
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. Did you consider making this a type alias instead? i.e.
Contributor
Author
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. I did and did have that type alias until just before pushing the PR. I'm not sure I like type aliases that just rename the type, since it's just an extra level of indirection to work out what is going on. Happy to put it back in if you feel strongly about it. |
||
|
|
||
| impl EventCountTags { | ||
| #[must_use] | ||
| pub fn new_empty() -> Self { | ||
| Self { | ||
| source: OptionalTag::Specified(None), | ||
| service: OptionalTag::Specified(None), | ||
| } | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn new_unspecified() -> Self { | ||
| Self { | ||
| source: OptionalTag::Ignored, | ||
| service: OptionalTag::Ignored, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Must be implemented by events to get the tags that will be attached to | ||
| /// the `component_sent_event_*` emitted metrics. | ||
| pub trait GetEventCountTags { | ||
| fn get_tags(&self) -> EventCountTags; | ||
| fn get_tags(&self) -> TaggedEventsSent; | ||
| } | ||
|
|
||
| /// Keeps track of the estimated json size of a given batch of events by | ||
|
|
@@ -48,7 +22,7 @@ pub enum GroupedCountByteSize { | |
| /// When we need to keep track of the events by certain tags we use this | ||
| /// variant. | ||
| Tagged { | ||
| sizes: HashMap<EventCountTags, CountByteSize>, | ||
| sizes: HashMap<TaggedEventsSent, CountByteSize>, | ||
| }, | ||
| /// If we don't need to track the events by certain tags we can use | ||
| /// this variant to avoid allocating a `HashMap`, | ||
|
|
@@ -86,7 +60,7 @@ impl GroupedCountByteSize { | |
| /// Returns `None` if we are not tracking by tags. | ||
| #[must_use] | ||
| #[cfg(test)] | ||
| pub fn sizes(&self) -> Option<&HashMap<EventCountTags, CountByteSize>> { | ||
| pub fn sizes(&self) -> Option<&HashMap<TaggedEventsSent, CountByteSize>> { | ||
| match self { | ||
| Self::Tagged { sizes } => Some(sizes), | ||
| Self::Untagged { .. } => None, | ||
|
|
@@ -131,7 +105,7 @@ impl GroupedCountByteSize { | |
| /// Emits our counts to a `RegisteredEvent` cached event. | ||
| pub fn emit_event<T, H>(&self, event_cache: &RegisteredEventCache<(), T>) | ||
| where | ||
| T: RegisterTaggedInternalEvent<Tags = EventCountTags, Fixed = (), Handle = H>, | ||
| T: RegisterTaggedInternalEvent<Tags = TaggedEventsSent, Fixed = (), Handle = H>, | ||
| H: InternalEventHandle<Data = CountByteSize>, | ||
| { | ||
| match self { | ||
|
|
@@ -141,7 +115,7 @@ impl GroupedCountByteSize { | |
| } | ||
| } | ||
| GroupedCountByteSize::Untagged { size } => { | ||
| event_cache.emit(&EventCountTags::new_unspecified(), *size); | ||
| event_cache.emit(&TaggedEventsSent::new_unspecified(), *size); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -177,21 +151,21 @@ impl<'a> Add<&'a GroupedCountByteSize> for GroupedCountByteSize { | |
|
|
||
| // The following two scenarios shouldn't really occur in practice, but are provided for completeness. | ||
| (Self::Tagged { mut sizes }, Self::Untagged { size }) => { | ||
| match sizes.get_mut(&EventCountTags::new_empty()) { | ||
| match sizes.get_mut(&TaggedEventsSent::new_empty()) { | ||
| Some(empty_size) => *empty_size += *size, | ||
| None => { | ||
| sizes.insert(EventCountTags::new_empty(), *size); | ||
| sizes.insert(TaggedEventsSent::new_empty(), *size); | ||
| } | ||
| } | ||
|
|
||
| Self::Tagged { sizes } | ||
| } | ||
| (Self::Untagged { size }, Self::Tagged { sizes }) => { | ||
| let mut sizes = sizes.clone(); | ||
| match sizes.get_mut(&EventCountTags::new_empty()) { | ||
| match sizes.get_mut(&TaggedEventsSent::new_empty()) { | ||
| Some(empty_size) => *empty_size += size, | ||
| None => { | ||
| sizes.insert(EventCountTags::new_empty(), size); | ||
| sizes.insert(TaggedEventsSent::new_empty(), size); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -307,6 +281,10 @@ pub trait MetaDescriptive { | |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use std::sync::Arc; | ||
|
|
||
| use crate::{config::ComponentKey, internal_event::OptionalTag}; | ||
|
|
||
| use super::*; | ||
|
|
||
| struct DummyEvent { | ||
|
|
@@ -315,8 +293,8 @@ mod tests { | |
| } | ||
|
|
||
| impl GetEventCountTags for DummyEvent { | ||
| fn get_tags(&self) -> EventCountTags { | ||
| EventCountTags { | ||
| fn get_tags(&self) -> TaggedEventsSent { | ||
| TaggedEventsSent { | ||
| source: self.source.clone(), | ||
| service: self.service.clone(), | ||
| } | ||
|
|
@@ -380,14 +358,14 @@ mod tests { | |
| assert_eq!( | ||
| vec![ | ||
| ( | ||
| EventCountTags { | ||
| TaggedEventsSent { | ||
| source: OptionalTag::Ignored, | ||
| service: Some("cabbage".to_string()).into() | ||
| }, | ||
| CountByteSize(2, JsonSize::new(78)) | ||
| ), | ||
| ( | ||
| EventCountTags { | ||
| TaggedEventsSent { | ||
| source: OptionalTag::Ignored, | ||
| service: Some("tomato".to_string()).into() | ||
| }, | ||
|
|
||
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 there a motivation to prefer one over the other? I don't disagree with this change, just wondering.
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.
Ah. I was experimenting with removing the
Ordand then just didn't change this back when I realised I needed theOrdanyway. I can't think of any clear reason why one would be better than the other.