Skip to content
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.

Sign up for GitHub

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

telemetry worker: flush data after stops, and two other fixes #515

Merged
merged 6 commits into from
Dec 6, 2024
Merged

telemetry worker: flush data after stops, and two other fixes #515

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
363d27d
telemetry worker: flush data after stops
cataphract Jul 1, 2024
b80b607
Fix configurations, integrations, deps being relayed over and over
cataphract Dec 5, 2024
b439363
Fix actions not being relayed in some circumstances
cataphract Dec 5, 2024
6b45e4a
Alternative method to prevent repetition of confs, integrations, deps
cataphract Dec 5, 2024
67da02b
Add comment
cataphract Dec 5, 2024
cbce086
Merge branch 'main' into glopes/flush-data-after-stop
cataphract Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions ddtelemetry/src/worker/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,10 @@ impl TelemetryWorker {
Lifecycle(Start) => {
if !self.data.started {
self.deadlines
.schedule_event(LifecycleAction::FlushData)
.schedule_event(LifecycleAction::FlushMetricAggr)
.unwrap();
self.deadlines
.schedule_event(LifecycleAction::FlushMetricAggr)
.schedule_event(LifecycleAction::FlushData)
.unwrap();
self.data.started = true;
}
Expand All @@ -265,7 +265,7 @@ impl TelemetryWorker {
.unwrap();
}
Lifecycle(FlushData) => {
if !self.data.started {
if !(self.data.started || self.config.restartable) {
return CONTINUE;
}
let batch = self.build_observability_batch();
Expand Down Expand Up @@ -296,7 +296,9 @@ impl TelemetryWorker {
self.log_err(&e);
}
self.data.started = false;
self.deadlines.clear_pending();
if !self.config.restartable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be enough to only include self.data.started = false; inside the if statement as well, and leave the exit early checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends. Do we want Start, Stop, Stop to generate two stops? Because that's what PHP ends up generating. The second stop is a noop, but if I moved the assignment self.data.started = false under the condition if !self.config.restartable, then the stops would be effective

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the exit early checks are still in the code, then how would the second Stop be effective?

Copy link
Contributor Author

@cataphract cataphract Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, what you're proposing is that I reset started = false only if !restartable. In that case started would stay true forever once there is a start. So the early check

 if !self.data.started {
   return BREAK;
 }

would never be hit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I was confusing the early checks.

There is something in the logic that feels a bit broken. The FlushData is also protected by this !self.data.started check. Should it work after a restartable stop?

The things that Stop does, should they happen when the first request ends, or when the worker stops?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should [FlushData] work after a restartable stop?

My guess is yes, otherwise there is no way to send the data that's collected after the stop (build_observability_batch is called only from the handlers of Stop and FlushData), at least not without an intervening start+stop.

The things that Stop does, should they happen when the first request ends, or when the worker stops?

That is a good point. The final flush of the metrics should happen when the worker stops, not when handling Stop. I guess at some point they were the same, but then the restart thing was introduced. But regardless, once we have a way to send metrics after a Stop, for that happen periodic flushes should still happen. So FlushData shouldn't be skipped or unscheduled after a Stop.

self.deadlines.clear_pending();
}
return BREAK;
}
CollectStats(stats_sender) => {
Expand Down Expand Up @@ -341,10 +343,10 @@ impl TelemetryWorker {
Err(err) => self.log_err(&err),
}
self.deadlines
.schedule_event(LifecycleAction::FlushData)
.schedule_event(LifecycleAction::FlushMetricAggr)
cataphract marked this conversation as resolved.
Show resolved Hide resolved
.unwrap();
self.deadlines
.schedule_event(LifecycleAction::FlushMetricAggr)
.schedule_event(LifecycleAction::FlushData)
.unwrap();
self.data.started = true;
}
Expand All @@ -368,7 +370,7 @@ impl TelemetryWorker {
.unwrap();
}
Lifecycle(FlushData) => {
if !self.data.started {
if !(self.data.started || self.config.restartable) {
return CONTINUE;
}
let mut batch = self.build_app_events_batch();
Expand Down Expand Up @@ -458,7 +460,9 @@ impl TelemetryWorker {
.await;

self.data.started = false;
self.deadlines.clear_pending();
if !self.config.restartable {
bantonsson marked this conversation as resolved.
Show resolved Hide resolved
self.deadlines.clear_pending();
}
return BREAK;
}
CollectStats(stats_sender) => {
Expand All @@ -470,27 +474,32 @@ impl TelemetryWorker {
}

// Builds telemetry payloads containing lifecycle events
fn build_app_events_batch(&self) -> Vec<Payload> {
fn build_app_events_batch(&mut self) -> Vec<Payload> {
let mut payloads = Vec::new();

if self.data.dependencies.flush_not_empty() {
payloads.push(data::Payload::AppDependenciesLoaded(
data::AppDependenciesLoaded {
dependencies: self.data.dependencies.unflushed().cloned().collect(),
dependencies: self.data.dependencies.drain_unflushed().cloned().collect(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these already be removed on a successful send by the call to removed_flushed? Yeah the method name has a d too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a new commit that uses the existing method.

},
))
}
if self.data.integrations.flush_not_empty() {
payloads.push(data::Payload::AppIntegrationsChange(
data::AppIntegrationsChange {
integrations: self.data.integrations.unflushed().cloned().collect(),
integrations: self.data.integrations.drain_unflushed().cloned().collect(),
},
))
}
if self.data.configurations.flush_not_empty() {
payloads.push(data::Payload::AppClientConfigurationChange(
data::AppClientConfigurationChange {
configuration: self.data.configurations.unflushed().cloned().collect(),
configuration: self
.data
.configurations
.drain_unflushed()
.cloned()
.collect(),
},
))
}
Expand Down
6 changes: 6 additions & 0 deletions ddtelemetry/src/worker/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,12 @@ where
pub fn len_stored(&self) -> usize {
self.items.len()
}

pub fn drain_unflushed(&mut self) -> impl Iterator<Item = &T> {
self.unflushed
.drain(..)
.flat_map(|i| self.items.get_idx(i).map(|(k, _)| k))
}
}

impl<T> Extend<T> for Store<T>
Expand Down
7 changes: 7 additions & 0 deletions sidecar/src/service/sidecar_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,13 @@ impl SidecarInterface for SidecarServer {
}

app.telemetry.send_msgs(actions).await.ok();

let mut extracted_actions: Vec<TelemetryActions> = vec![];
enqueued_data
.extract_telemetry_actions(&mut extracted_actions)
.await;
app.telemetry.send_msgs(extracted_actions).await.ok();

// Ok, we dequeued all messages, now new enqueue_actions calls can handle it
completer.complete((service_name, env_name)).await;
}
Expand Down
Loading