chore: clean up eventd and service check types #130

tobz · 2024-07-22T23:53:51Z

Context

Just a little bit of cleanup to polish the code comments and organization of events/service checks, introduced in #129.

tobz · 2024-07-22T23:55:27Z

lib/saluki-event/src/eventd/mod.rs

+// TODO: Switch usages of `String` to `MetaString` since we should generally be able to intern these strings as they
+// originate in in the DogStatsD codec, where interning is already taking place.


Usually we'll try to keep TODO comments close to the area where they're most relevant, but in this case, this is a thing we'd want to apply to multiple fields on the EventD struct, so we just shove it up here as a general TODO.

tobz · 2024-07-23T00:03:41Z

lib/saluki-event/src/eventd/mod.rs

+// originate in in the DogStatsD codec, where interning is already taking place.
+
+/// Alert type.
+#[derive(Clone, Copy, Debug)]


While this was previously only Clone, enums are a prime candidate for being Copy. This means the value can be copied directly -- just copy the raw memory bytes and nothing else -- without any special logic.

Generally, integral types (signed and unsigned integers, floats, booleans) and "unit" enums (the variants don't have any fields) can stand to be Copy because their size in memory is no bigger than a reference to the type itself.

This is to say that if you compare the size of this enum to a reference to this enum, the reference to it is much larger: 8 bytes versus 1 byte. References are at least a machine word (so 8 bytes on 64-bit architectures) while the compiler ends up bitpacking this enum in a single byte. The same thing holds if we use an integer type like u32: &u32 is 8 bytes while u32 itself is only 4 bytes.

tobz · 2024-07-23T00:04:21Z

lib/saluki-event/src/eventd/mod.rs

@@ -13,55 +37,57 @@ pub struct EventD {
    timestamp: u64,
    host: String,
    aggregation_key: String,
-    priority: EventPriority,
+    priority: Priority,


We've renamed EventPriority to Priority to avoid stuttering.

tobz · 2024-07-23T00:04:37Z

lib/saluki-event/src/eventd/mod.rs

    source_type_name: String,
-    alert_type: EventAlertType,
+    alert_type: AlertType,


Similarly, we've renamed EventAlertType to AlertType to avoid stuttering.

pr-commenter · 2024-07-23T00:26:35Z

Regression Detector (DogStatsD)

Regression Detector Results

Run ID: 8a8f939e-44d9-4ce2-8e89-1f1c24dff04b

Baseline: 7.52.0
Comparison: 7.52.1

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	dsd_uds_1mb_3k_contexts	ingress throughput	+0.02	[-0.04, +0.08]
➖	dsd_uds_500mb_3k_contexts	ingress throughput	+0.00	[+0.00, +0.00]
➖	dsd_uds_100mb_3k_contexts	ingress throughput	+0.00	[-0.01, +0.01]
➖	dsd_uds_1mb_50k_contexts	ingress throughput	+0.00	[-0.00, +0.00]
➖	dsd_uds_1mb_50k_contexts_memlimit	ingress throughput	-0.00	[-0.00, +0.00]
➖	dsd_uds_100mb_250k_contexts	ingress throughput	-0.01	[-0.04, +0.02]
➖	dsd_uds_512kb_3k_contexts	ingress throughput	-0.02	[-0.09, +0.06]
➖	dsd_uds_10mb_3k_contexts	ingress throughput	-0.03	[-0.05, -0.00]
➖	dsd_uds_100mb_3k_contexts_distributions_only	memory utilization	-0.86	[-1.07, -0.65]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

pr-commenter · 2024-07-23T00:27:38Z

Regression Detector (Saluki)

Regression Detector Results

Run ID: 9ecb1e0c-b352-4d75-a4f7-bc14fbd6aa4d

Baseline: 8612664
Comparison: 64adfd0

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	dsd_uds_100mb_3k_contexts_distributions_only	memory utilization	+1.26	[+1.11, +1.41]
➖	dsd_uds_500mb_3k_contexts	ingress throughput	+0.74	[+0.61, +0.86]
➖	dsd_uds_50mb_10k_contexts_no_inlining	ingress throughput	+0.01	[-0.06, +0.07]
➖	dsd_uds_100mb_3k_contexts	ingress throughput	+0.01	[-0.01, +0.02]
➖	dsd_uds_50mb_10k_contexts_no_inlining_no_allocs	ingress throughput	-0.01	[-0.08, +0.06]
➖	dsd_uds_512kb_3k_contexts	ingress throughput	-0.01	[-0.21, +0.19]
➖	dsd_uds_10mb_3k_contexts	ingress throughput	-0.02	[-0.10, +0.06]
➖	dsd_uds_1mb_50k_contexts	ingress throughput	-0.03	[-0.06, +0.01]
➖	dsd_uds_1mb_3k_contexts	ingress throughput	-0.05	[-0.25, +0.14]
➖	dsd_uds_100mb_250k_contexts	ingress throughput	-0.19	[-0.41, +0.04]
➖	dsd_uds_1mb_50k_contexts_memlimit	ingress throughput	-0.85	[-3.20, +1.50]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

…reality

tobz · 2024-07-23T00:34:23Z

lib/saluki-core/src/topology/graph.rs

-            .with_source("in_log", DataType::Log)
-            .with_destination("out_log", DataType::Log)
-            .with_edge_fallible("in log", "out_log")
+            .with_source("in_eventd", DataType::EventD)


This, and all changes in this file, are just renaming to reflect the fact I removed DataType::Log and DataType::Trace.

tobz · 2024-07-23T00:37:08Z

lib/saluki-event/src/eventd/mod.rs

    tags: Vec<String>,
 }

 impl EventD {
-    /// Gets a reference to the title.
+    /// Returns the title of the event.


This change in wording (like across the board for the fields in this type) is a little bit nit-picky, and I know the existing codebase has a lot of Gets a reference to X, but that's a pattern I want to clean up in the future.

In general, for accessors, this is the format I'd like to use for doc comments, since it better reflects the common wording for accessors, which is Returns ....

We also do have some stuttering here -- the of the event suffix for each comment -- but this is generally the only place where I think it's OK, because stuttering in types is always in your face since it's in the code, whereas in comments, it's hidden until you... look at the docs, or hover over a type and get intellisense, etc.

Not a hard and fast rule, but just trying to solidify the style here since we don't (yet) have a style guide.

tobz · 2024-07-23T00:38:05Z

lib/saluki-event/src/eventd/mod.rs

+
+/// Event priority.
+#[derive(Clone, Copy, Debug)]
+pub enum Priority {


For both Priority and AlertType, I've moved them to mod.rs because they're simple enough that it ends up being a little cleaner to have it all in the same file. If we had an impl block, or tests, then it might make sense to push it into a separate file.

tobz · 2024-07-23T00:38:58Z

lib/saluki-event/src/lib.rs

+    /// Datadog Events.
+    EventD,

-    /// Traces.
-    Trace,
+    /// Service checks.
+    ServiceCheck,


The old variants -- Log and Trace -- didn't actually map to any possible variant in Event, so I took this as a good chance to align it with Event.

tobz · 2024-07-23T00:41:22Z

lib/saluki-event/src/lib.rs

@@ -25,11 +25,11 @@ pub enum DataType {
    /// Metrics.
    Metric,

-    /// Logs.
-    Log,
+    /// Datadog Events.


In general, Saluki wants to be as agnostic as it can be: we're providing building blocks to making something like the Agent but we don't want to necessarily write as if it's just the agent... so we'll keep names/descriptions generic where possible, and then specialize (like calling these "Datadog Events") where needed to disambiguate.

tobz · 2024-07-23T00:51:31Z

lib/saluki-event/src/lib.rs

+    /// Returns a mutable reference inner event value, if this event is a `Metric`.
    ///
-    /// If the underlying event is not a `Metric`, this will return `None`.
-    pub fn as_metric_mut(&mut self) -> Option<&mut Metric> {
+    /// Otherwise, `None` is returned.
+    pub fn try_as_metric_mut(&mut self) -> Option<&mut Metric> {
        match self {
            Event::Metric(metric) => Some(metric),
            _ => None,
        }
    }

-    /// Converts this event into `Eventd`.
+    /// Returns the inner event value, if this event is a `EventD`.
    ///
-    /// If the underlying event is not a `Eventd`, this will return `None`.
-    pub fn into_eventd(self) -> Option<EventD> {
+    /// Otherwise, `None` is returned and the original event is consumed.
+    pub fn try_into_eventd(self) -> Option<EventD> {
        match self {
            Event::EventD(eventd) => Some(eventd),
            _ => None,
        }
    }


I updated the name of these methods because I gave you a bad suggestion in the original PR.

The common naming practice for conversion methods in Rust is:

into_foo if it consumes the value and the conversion isn't fallible (i.e. no Option<T> or Result<T, E>)

as_foo if it provides an immutable reference conversion (i.e. String::as_str returns &str)

as_foo_mut if it provides a mutable reference conversion (i.e. Vec::as_mut_slice returns &mut [T])

try_into_foo/try_as_foo/try_as_foo_mut if the conversion is fallible (i.e. how we're doing it because we don't know if an Event is actually a Metric or not)

tobz · 2024-07-23T00:53:24Z

lib/saluki-event/src/service_check/mod.rs

        &self.message
    }

-    /// Gets a reference to the tags
-    pub fn tags(&self) -> &Vec<String> {
+    /// Returns the tags associated with the check.


Also, super small style nit: we always write comments as full sentences, including ending with a period.

chore: clean up eventd and service check types

84e7620

tobz requested a review from a team as a code owner July 22, 2024 23:53

github-actions bot added the area/core Core functionality, event model, etc. label Jul 22, 2024

tobz commented Jul 22, 2024

View reviewed changes

tobz commented Jul 23, 2024

View reviewed changes

tobz added 2 commits July 23, 2024 00:32

update the wording for accessors

dca01a7

update naming for event conversion methods + fix DataType to reflect …

033bcd2

…reality

github-actions bot added area/components Sources, transforms, and destinations. source/dogstatsd DogStatsD source. transform/host-enrichment Host Enrichment synchronous transform. transform/origin-enrichment Origin Enrichment synchronous transform. labels Jul 23, 2024

tobz commented Jul 23, 2024

View reviewed changes

missed a spot

64adfd0

github-actions bot added transform/aggregate Aggregate transform. destination/datadog-metrics Datadog Metrics destination. destination/prometheus Prometheus Scrape destination. labels Jul 23, 2024

tobz commented Jul 23, 2024

View reviewed changes

rayz approved these changes Jul 23, 2024

View reviewed changes

tobz merged commit adf91b2 into main Jul 23, 2024
13 checks passed

tobz deleted the tobz/clean-up-eventd-service-check-types branch July 23, 2024 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: clean up eventd and service check types #130

chore: clean up eventd and service check types #130

tobz commented Jul 22, 2024

tobz Jul 22, 2024

tobz Jul 23, 2024 •

edited

Loading

tobz Jul 23, 2024

tobz Jul 23, 2024

pr-commenter bot commented Jul 23, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

pr-commenter bot commented Jul 23, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

tobz Jul 23, 2024

tobz Jul 23, 2024 •

edited

Loading

tobz Jul 23, 2024

tobz Jul 23, 2024

tobz Jul 23, 2024

tobz Jul 23, 2024

tobz Jul 23, 2024 •

edited

Loading

		// TODO: Switch usages of `String` to `MetaString` since we should generally be able to intern these strings as they
		// originate in in the DogStatsD codec, where interning is already taking place.

chore: clean up eventd and service check types #130

chore: clean up eventd and service check types #130

Conversation

tobz commented Jul 22, 2024

Context

Choose a reason for hiding this comment

tobz Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pr-commenter bot commented Jul 23, 2024 • edited Loading

Regression Detector (DogStatsD)

Regression Detector Results

No significant changes in experiment optimization goals

Fine details of change detection per experiment

Explanation

pr-commenter bot commented Jul 23, 2024 • edited Loading

Regression Detector (Saluki)

Regression Detector Results

No significant changes in experiment optimization goals

Fine details of change detection per experiment

Explanation

Choose a reason for hiding this comment

tobz Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobz Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

tobz Jul 23, 2024 •

edited

Loading

pr-commenter bot commented Jul 23, 2024 •

edited

Loading

pr-commenter bot commented Jul 23, 2024 •

edited

Loading

tobz Jul 23, 2024 •

edited

Loading

tobz Jul 23, 2024 •

edited

Loading