Skip to content

Commit 48ae5a3

Browse files
nikhilsinhaparseablenikhilsinhaparseable
authored
bug fixes: alerts and ingestion script (#1746)
* bug fixes: alerts and ingestion script 1. load alerts at server startup 2. delete alert for localfs 3. ingestion script - start and stop path incorrect * retry and kill
1 parent e32c0e9 commit 48ae5a3

5 files changed

Lines changed: 205 additions & 38 deletions

File tree

‎scripts/ingest.ps1‎

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,23 @@ $ProgressPreference = 'SilentlyContinue'
1919
$COLLECTOR_VERSION = "0.157.0"
2020
$INSTALL_DIR = "$env:LOCALAPPDATA\parseable-otelcol"
2121
$COLLECTOR_EXE = "$INSTALL_DIR\otelcol.exe"
22-
$CONFIG_FILE = "$PSScriptRoot\otelcol.yaml"
23-
$PID_FILE = "$PSScriptRoot\otelcol.pid"
24-
$LOG_FILE = "$PSScriptRoot\otelcol.log"
25-
$ERROR_LOG_FILE = "$PSScriptRoot\otelcol.err.log"
22+
$SCRIPT_DIR = if ([string]::IsNullOrWhiteSpace($PSScriptRoot)) {
23+
(Get-Location).Path
24+
}
25+
else {
26+
$PSScriptRoot
27+
}
28+
$CONFIG_FILE = Join-Path $SCRIPT_DIR "otelcol.yaml"
29+
$PID_FILE = Join-Path $SCRIPT_DIR "otelcol.pid"
30+
$LOG_FILE = Join-Path $SCRIPT_DIR "otelcol.log"
31+
$ERROR_LOG_FILE = Join-Path $SCRIPT_DIR "otelcol.err.log"
2632

2733
$SCRIPT_PATH = $PSCommandPath
2834
if ([string]::IsNullOrWhiteSpace($SCRIPT_PATH)) {
2935
$SCRIPT_PATH = $MyInvocation.MyCommand.Path
3036
}
3137
if ([string]::IsNullOrWhiteSpace($SCRIPT_PATH)) {
32-
$SCRIPT_PATH = Join-Path $PSScriptRoot "ingest.ps1"
38+
$SCRIPT_PATH = Join-Path $SCRIPT_DIR "ingest.ps1"
3339
}
3440
$SCRIPT_CMD = "powershell -NoProfile -ExecutionPolicy Bypass -File '$SCRIPT_PATH'"
3541

@@ -48,6 +54,11 @@ function Write-ErrorMsg {
4854
Write-Host "[ERROR] $Message" -ForegroundColor Red
4955
}
5056

57+
function ConvertTo-PowerShellSingleQuoted {
58+
param([string]$Value)
59+
return "'" + $Value.Replace("'", "''") + "'"
60+
}
61+
5162
function Write-ParseableBanner {
5263
$accent = "$([char]27)[38;2;158;158;240m"
5364
$reset = "$([char]27)[0m"
@@ -135,9 +146,19 @@ function Stop-Collector {
135146

136147
try {
137148
Stop-Process -Id $processId -Force -ErrorAction Stop
138-
Start-Sleep -Seconds 2
149+
for ($attempt = 0; $attempt -lt 10; $attempt++) {
150+
if ($null -eq (Get-Process -Id $processId -ErrorAction SilentlyContinue)) {
151+
break
152+
}
153+
Start-Sleep -Seconds 1
154+
}
155+
156+
if ($null -ne (Get-Process -Id $processId -ErrorAction SilentlyContinue)) {
157+
throw "Process $processId did not stop within 10 seconds; PID file was preserved."
158+
}
159+
160+
Remove-Item $PID_FILE -Force -ErrorAction Stop
139161
Write-Info "OpenTelemetry Collector stopped successfully"
140-
Remove-Item $PID_FILE -ErrorAction SilentlyContinue
141162
}
142163
catch {
143164
Write-ErrorMsg "Failed to stop OpenTelemetry Collector: $_"
@@ -158,8 +179,10 @@ function Show-Status {
158179
Write-Info "Config file: $CONFIG_FILE"
159180
Write-Info "Log file: $LOG_FILE"
160181
Write-Info "Error log file: $ERROR_LOG_FILE"
161-
Write-Info "To see logs: $SCRIPT_CMD logs"
162-
Write-Info "To stop: $SCRIPT_CMD stop"
182+
$pidFileCommand = ConvertTo-PowerShellSingleQuoted $PID_FILE
183+
$errorLogFileCommand = ConvertTo-PowerShellSingleQuoted $ERROR_LOG_FILE
184+
Write-Info "To see logs: Get-Content $errorLogFileCommand -Tail 80 -Wait"
185+
Write-Info "To stop: Stop-Process -Id (Get-Content $pidFileCommand); Remove-Item $pidFileCommand"
163186
}
164187
else {
165188
Write-Warning "OpenTelemetry Collector is not running"
@@ -292,15 +315,18 @@ function Start-Collector {
292315
$stillRunning = Get-Process -Id $process.Id -ErrorAction SilentlyContinue
293316
if (-not $stillRunning) {
294317
Write-ErrorMsg "OpenTelemetry Collector exited immediately"
295-
Write-ErrorMsg "Run '$SCRIPT_CMD logs' to see error details"
318+
$errorLogFileCommand = ConvertTo-PowerShellSingleQuoted $ERROR_LOG_FILE
319+
Write-ErrorMsg "Check logs: Get-Content $errorLogFileCommand -Tail 80"
296320
Remove-Item $PID_FILE -ErrorAction SilentlyContinue
297321
exit 1
298322
}
299323

324+
$pidFileCommand = ConvertTo-PowerShellSingleQuoted $PID_FILE
325+
$errorLogFileCommand = ConvertTo-PowerShellSingleQuoted $ERROR_LOG_FILE
300326
Write-Info "OpenTelemetry Collector started successfully (PID: $($process.Id))"
301-
Write-Info "To check status: $SCRIPT_CMD status"
302-
Write-Info "To see logs: $SCRIPT_CMD logs"
303-
Write-Info "To stop: $SCRIPT_CMD stop"
327+
Write-Info "To check status: Get-Process -Id (Get-Content $pidFileCommand)"
328+
Write-Info "To see logs: Get-Content $errorLogFileCommand -Tail 80 -Wait"
329+
Write-Info "To stop: Stop-Process -Id (Get-Content $pidFileCommand); Remove-Item $pidFileCommand"
304330
return $true
305331
}
306332

@@ -432,7 +458,9 @@ function Setup-Collector {
432458

433459
$configContent = ($configLines -join [Environment]::NewLine) + [Environment]::NewLine
434460
$utf8NoBom = New-Object System.Text.UTF8Encoding $false
435-
$tempConfigFile = "$CONFIG_FILE.$([guid]::NewGuid().ToString('N')).tmp"
461+
$configUpdateId = [guid]::NewGuid().ToString('N')
462+
$tempConfigFile = "$CONFIG_FILE.$configUpdateId.tmp"
463+
$backupConfigFile = "$CONFIG_FILE.$configUpdateId.bak"
436464

437465
try {
438466
$tempConfigStream = [System.IO.File]::Create($tempConfigFile)
@@ -462,15 +490,20 @@ function Setup-Collector {
462490

463491
if (Test-Path $CONFIG_FILE) {
464492
Set-Acl -Path $CONFIG_FILE -AclObject $acl -ErrorAction Stop
465-
[System.IO.File]::Replace($tempConfigFile, $CONFIG_FILE, $null)
493+
[System.IO.File]::Replace($tempConfigFile, $CONFIG_FILE, $backupConfigFile)
466494
}
467495
else {
468496
[System.IO.File]::Move($tempConfigFile, $CONFIG_FILE)
469497
}
470498
Set-Acl -Path $CONFIG_FILE -AclObject $acl -ErrorAction Stop
471499
}
500+
catch {
501+
Write-ErrorMsg "Failed to update OpenTelemetry Collector configuration: $_"
502+
exit 1
503+
}
472504
finally {
473505
Remove-Item $tempConfigFile -Force -ErrorAction SilentlyContinue
506+
Remove-Item $backupConfigFile -Force -ErrorAction SilentlyContinue
474507
}
475508

476509
if (Test-CollectorRunning) {

‎scripts/ingest.sh‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,10 @@ stop_collector() {
8282
if is_running; then
8383
PID=$(cat "$PID_FILE")
8484
print_info "Stopping OpenTelemetry Collector (PID: $PID)..."
85-
kill "$PID"
85+
if ! kill "$PID"; then
86+
print_error "Failed to stop OpenTelemetry Collector; PID file was preserved"
87+
return 1
88+
fi
8689

8790
for _ in {1..10}; do
8891
if ! ps -p "$PID" > /dev/null 2>&1; then
@@ -93,11 +96,28 @@ stop_collector() {
9396
sleep 1
9497
done
9598

96-
if ps -p "$PID" > /dev/null 2>&1; then
99+
if is_running; then
97100
print_warning "Force killing OpenTelemetry Collector..."
98-
kill -9 "$PID"
99-
rm -f "$PID_FILE"
101+
if ! kill -9 "$PID"; then
102+
print_error "Failed to force stop OpenTelemetry Collector; PID file was preserved"
103+
return 1
104+
fi
105+
106+
for _ in {1..5}; do
107+
if ! ps -p "$PID" > /dev/null 2>&1; then
108+
rm -f "$PID_FILE"
109+
print_info "✓ OpenTelemetry Collector stopped successfully"
110+
return 0
111+
fi
112+
sleep 1
113+
done
114+
115+
print_error "OpenTelemetry Collector did not stop; PID file was preserved"
116+
return 1
100117
fi
118+
119+
rm -f "$PID_FILE"
120+
print_info "✓ OpenTelemetry Collector stopped successfully"
101121
else
102122
print_warning "OpenTelemetry Collector is not running"
103123
if [ -f "$PID_FILE" ]; then
@@ -256,8 +276,8 @@ start_collector() {
256276
if ps -p "$PID" > /dev/null 2>&1; then
257277
print_info "✓ OpenTelemetry Collector started successfully (PID: $PID)"
258278
print_info "View logs: tail -f $LOG_FILE"
259-
print_info "Check status: $0 status"
260-
print_info "Stop: $0 stop"
279+
print_info "Check status: ps -p \$(cat $PID_FILE) -o pid,ppid,user,%cpu,%mem,etime,command"
280+
print_info "Stop: kill \$(cat $PID_FILE) && rm -f $PID_FILE"
261281
else
262282
print_error "✗ OpenTelemetry Collector failed to start. Check logs: cat $LOG_FILE"
263283
rm -f "$PID_FILE"

‎src/alerts/mod.rs‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,10 @@ pub fn create_default_alerts_manager() -> Alerts {
169169
alerts
170170
}
171171

172+
fn alert_tenant_from_storage_key(tenant: &str) -> Option<String> {
173+
(!tenant.is_empty() && tenant != DEFAULT_TENANT).then(|| tenant.to_owned())
174+
}
175+
172176
pub async fn user_auth_for_alert_config(
173177
session: &SessionKey,
174178
alert: &AlertConfig,
@@ -1174,18 +1178,14 @@ impl AlertManagerTrait for Alerts {
11741178
let mut map = self.alerts.write().await;
11751179

11761180
for (tenant_id, raw_bytes) in raw_objects {
1177-
let tenant = if tenant_id.is_empty() {
1178-
&None
1179-
} else {
1180-
&Some(tenant_id.clone())
1181-
};
1181+
let tenant = alert_tenant_from_storage_key(&tenant_id);
11821182
for alert_bytes in raw_bytes {
1183-
let Some(mut alert) = parse_alert_config(&alert_bytes, tenant).await else {
1183+
let Some(mut alert) = parse_alert_config(&alert_bytes, &tenant).await else {
11841184
continue;
11851185
};
11861186

11871187
// ensure that alert config's tenant is correctly set
1188-
alert.tenant_id.clone_from(tenant);
1188+
alert.tenant_id.clone_from(&tenant);
11891189

11901190
let alert = match alert_from_config_oss(alert) {
11911191
Ok(alert) => alert,
@@ -1673,3 +1673,19 @@ fn get_severity_priority(severity: &Severity) -> u8 {
16731673
Severity::Low => 3,
16741674
}
16751675
}
1676+
1677+
#[cfg(test)]
1678+
mod tests {
1679+
use super::alert_tenant_from_storage_key;
1680+
use crate::parseable::DEFAULT_TENANT;
1681+
1682+
#[test]
1683+
fn default_storage_tenant_is_not_an_explicit_tenant() {
1684+
assert_eq!(alert_tenant_from_storage_key(""), None);
1685+
assert_eq!(alert_tenant_from_storage_key(DEFAULT_TENANT), None);
1686+
assert_eq!(
1687+
alert_tenant_from_storage_key("tenant-a"),
1688+
Some("tenant-a".to_owned())
1689+
);
1690+
}
1691+
}

‎src/metastore/metastores/object_store_metastore.rs‎

Lines changed: 106 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -325,11 +325,17 @@ impl Metastore for ObjectStoreMetastore {
325325
obj: &dyn MetastoreObject,
326326
tenant_id: &Option<String>,
327327
) -> Result<(), MetastoreError> {
328-
let path = obj.get_object_path();
329-
Ok(self
330-
.storage
331-
.delete_object(&RelativePathBuf::from(path), tenant_id)
332-
.await?)
328+
let id = Ulid::from_string(&obj.get_object_id()).map_err(|e| MetastoreError::Error {
329+
status_code: StatusCode::BAD_REQUEST,
330+
message: e.to_string(),
331+
flow: "delete_alert".into(),
332+
})?;
333+
let path = alert_json_path(id, tenant_id);
334+
match self.storage.delete_object(&path, tenant_id).await {
335+
Ok(()) => Ok(()),
336+
Err(err) if is_missing_optional_dir(&err) => Ok(()),
337+
Err(err) => Err(MetastoreError::ObjectStorageError(err)),
338+
}
333339
}
334340

335341
/// alerts state
@@ -439,11 +445,17 @@ impl Metastore for ObjectStoreMetastore {
439445
obj: &dyn MetastoreObject,
440446
tenant_id: &Option<String>,
441447
) -> Result<(), MetastoreError> {
442-
let path = obj.get_object_path();
443-
Ok(self
444-
.storage
445-
.delete_object(&RelativePathBuf::from(path), tenant_id)
446-
.await?)
448+
let id = Ulid::from_string(&obj.get_object_id()).map_err(|e| MetastoreError::Error {
449+
status_code: StatusCode::BAD_REQUEST,
450+
message: e.to_string(),
451+
flow: "delete_alert_state".into(),
452+
})?;
453+
let path = alert_state_json_path(id, tenant_id);
454+
match self.storage.delete_object(&path, tenant_id).await {
455+
Ok(()) => Ok(()),
456+
Err(err) if is_missing_optional_dir(&err) => Ok(()),
457+
Err(err) => Err(MetastoreError::ObjectStorageError(err)),
458+
}
447459
}
448460

449461
/// Get MTTR history from storage
@@ -1506,3 +1518,87 @@ impl Metastore for ObjectStoreMetastore {
15061518
}
15071519
}
15081520
}
1521+
1522+
#[cfg(test)]
1523+
mod tests {
1524+
use super::*;
1525+
use crate::storage::LocalFS;
1526+
1527+
#[derive(serde::Serialize)]
1528+
struct TestMetastoreObject {
1529+
id: Ulid,
1530+
path: String,
1531+
}
1532+
1533+
impl MetastoreObject for TestMetastoreObject {
1534+
fn get_object_path(&self) -> String {
1535+
self.path.clone()
1536+
}
1537+
1538+
fn get_object_id(&self) -> String {
1539+
self.id.to_string()
1540+
}
1541+
}
1542+
1543+
#[tokio::test]
1544+
async fn delete_alert_uses_request_tenant_path_and_is_idempotent() {
1545+
let temp_dir = tempfile::tempdir().unwrap();
1546+
let storage = Arc::new(LocalFS::new(temp_dir.path().to_path_buf()));
1547+
let metastore = ObjectStoreMetastore {
1548+
storage: storage.clone(),
1549+
};
1550+
let id = Ulid::new();
1551+
let tenant_id = None;
1552+
let path = alert_json_path(id, &tenant_id);
1553+
storage
1554+
.put_object(&path, Bytes::from_static(b"{}"), &tenant_id)
1555+
.await
1556+
.unwrap();
1557+
let object = TestMetastoreObject {
1558+
id,
1559+
path: format!("{DEFAULT_TENANT}/{ALERTS_ROOT_DIRECTORY}/{id}.json"),
1560+
};
1561+
1562+
metastore.delete_alert(&object, &tenant_id).await.unwrap();
1563+
metastore.delete_alert(&object, &tenant_id).await.unwrap();
1564+
1565+
assert!(matches!(
1566+
storage.get_object(&path, &tenant_id).await,
1567+
Err(ObjectStorageError::NoSuchKey(_))
1568+
));
1569+
}
1570+
1571+
#[tokio::test]
1572+
async fn delete_alert_state_uses_request_tenant_path_and_is_idempotent() {
1573+
let temp_dir = tempfile::tempdir().unwrap();
1574+
let storage = Arc::new(LocalFS::new(temp_dir.path().to_path_buf()));
1575+
let metastore = ObjectStoreMetastore {
1576+
storage: storage.clone(),
1577+
};
1578+
let id = Ulid::new();
1579+
let tenant_id = None;
1580+
let path = alert_state_json_path(id, &tenant_id);
1581+
storage
1582+
.put_object(&path, Bytes::from_static(b"{}"), &tenant_id)
1583+
.await
1584+
.unwrap();
1585+
let object = TestMetastoreObject {
1586+
id,
1587+
path: format!("{DEFAULT_TENANT}/{ALERTS_ROOT_DIRECTORY}/alert_state_{id}.json"),
1588+
};
1589+
1590+
metastore
1591+
.delete_alert_state(&object, &tenant_id)
1592+
.await
1593+
.unwrap();
1594+
metastore
1595+
.delete_alert_state(&object, &tenant_id)
1596+
.await
1597+
.unwrap();
1598+
1599+
assert!(matches!(
1600+
storage.get_object(&path, &tenant_id).await,
1601+
Err(ObjectStorageError::NoSuchKey(_))
1602+
));
1603+
}
1604+
}

‎src/storage/mod.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ mod azure_blob;
4343
pub mod field_stats;
4444
mod gcs;
4545
mod localfs;
46+
#[cfg(test)]
47+
pub(crate) use localfs::LocalFS;
4648
mod metrics_layer;
4749
pub mod object_storage;
4850
pub mod retention;

0 commit comments

Comments
 (0)