[Synthetics] Technical debt in synthetics_private_location.ts - unused params, magic numbers, silent failures, and O(n×m×s) complexity

[dominiqueclarke]

Summary

The synthetics_private_location.ts file has several pre-existing code quality and robustness issues that should be addressed. These are not regressions but technical debt that could cause problems at scale or lead to silent failures.

---

Issue 1: Unused _spaceId parameter in getPolicyId (Line 79)

Current Code

getPolicyId(config: { origin?: string; id: string }, locId: string, _spaceId?: string) {
  return `${config.id}-${locId}`;
}

Problem

The _spaceId parameter is accepted but never used. This is confusing for callers who might assume it affects the generated ID.

Suggestion

Remove the unused parameter or document why it exists (e.g., for API compatibility):

// Option A: Remove unused parameter
getPolicyId(config: { origin?: string; id: string }, locId: string) {
  return `${config.id}-${locId}`;
}

// Option B: Document if kept for compatibility
/**
 * @param _spaceId - Deprecated, kept for call-site compatibility. Space is no longer part of policy ID.
 */
getPolicyId(config: { origin?: string; id: string }, locId: string, _spaceId?: string) {
  return `${config.id}-${locId}`;
}

---

Issue 2: Magic number in space aggregation (Line 117)

Current Code

aggs: {
  spaces: {
    terms: {
      field: `${syntheticsMonitorSavedObjectType}.namespaces`,
      size: 1000,  // Magic number
    },
  },
},

Problem

• Hard-coded size: 1000 could miss spaces in large deployments
• No warning if limit is exceeded
• Magic number without explanation

Suggestions

// Option A: Use a named constant with documentation
const MAX_SPACES_AGGREGATION = 1000; // ES terms aggregation limit

aggs: {
  spaces: {
    terms: {
      field: `${syntheticsMonitorSavedObjectType}.namespaces`,
      size: MAX_SPACES_AGGREGATION,
    },
  },
},

// Option B: Add runtime check for truncation
const spacesAgg = response.aggregations?.spaces as { buckets: Array<{ key: string }> };
if (spacesAgg?.buckets?.length === MAX_SPACES_AGGREGATION) {
  this.server.logger.warn(
    `Space aggregation may be truncated. Found ${MAX_SPACES_AGGREGATION} spaces, there may be more.`
  );
}

---

Issue 3: Silent failure in getAllSpacesWithMonitors (Lines 131-133)

Current Code

async getAllSpacesWithMonitors(): Promise<string[]> {
  try {
    // ... aggregation query ...
    return spacesAgg?.buckets?.map((bucket) => bucket.key) ?? [];
  } catch (e) {
    this.server.logger.error(e);
    return [];  // Silent failure - returns empty array
  }
}

Problem

When the aggregation fails:

• Returns empty array instead of throwing
• Callers assume there are no other spaces with monitors
• Legacy policies in other spaces won't be found for cleanup/migration
• Could lead to orphaned policies or duplicate policies

Suggestions

// Option A: Let errors propagate for callers to handle
async getAllSpacesWithMonitors(): Promise<string[]> {
  // Remove try-catch, let errors propagate
  const response = await this.server.syntheticsEsClient.search(/* ... */);
  return spacesAgg?.buckets?.map((bucket) => bucket.key) ?? [];
}

// Option B: Return a result type that indicates success/failure
async getAllSpacesWithMonitors(): Promise<{ spaces: string[]; error?: Error }> {
  try {
    const response = await this.server.syntheticsEsClient.search(/* ... */);
    const spaces = spacesAgg?.buckets?.map((bucket) => bucket.key) ?? [];
    return { spaces };
  } catch (e) {
    this.server.logger.error(e);
    return { spaces: [], error: e as Error };
  }
}

// Option C: Fall back to current space only with warning
async getAllSpacesWithMonitors(currentSpaceId: string): Promise<string[]> {
  try {
    // ... existing logic ...
  } catch (e) {
    this.server.logger.warn(
      `Failed to get all spaces with monitors, falling back to current space only: ${e.message}`
    );
    return [currentSpaceId];
  }
}

---

Issue 4: O(n×m×s) complexity in getExistingPolicies (Lines 463-469)

Current Code

for (const config of configs) {              // n configs
  for (const privateLocation of allPrivateLocations) {  // m ALL locations
    policyIdsToFetch.add(this.getPolicyId(config, privateLocation.id));
    for (const space of allSpaces) {         // s spaces
      policyIdsToFetch.add(this.getLegacyPolicyId(config.id, privateLocation.id, space));
    }
  }
}

Problem

• Iterates ALL private locations, not just those used by the configs
• Generates speculative IDs for every possible combination
• At scale: 100 monitors × 10 locations × 50 spaces = 50,000+ IDs to fetch
• Most IDs won't exist (speculative lookup)

Suggestions

// Option A: Filter to relevant locations only (recommended - quick win)
for (const config of configs) {
  // Only iterate locations this config actually uses
  const configLocationIds = new Set(
    config.locations.filter(loc => !loc.isServiceManaged).map(loc => loc.id)
  );
  
  for (const privateLocation of allPrivateLocations) {
    if (!configLocationIds.has(privateLocation.id)) continue;
    
    policyIdsToFetch.add(this.getPolicyId(config, privateLocation.id));
    for (const space of allSpaces) {
      policyIdsToFetch.add(this.getLegacyPolicyId(config.id, privateLocation.id, space));
    }
  }
}

// Option B: Query-based lookup instead of speculative IDs
const monitorIds = configs.map(c => c.id);
const existingPolicies = await this.server.fleet.packagePolicyService.list(soClient, {
  kuery: monitorIds.map(id => `ingest-package-policies.id:${id}*`).join(' OR '),
  perPage: 10000,
});

// Option C: Batch large ID sets to avoid timeouts
const BATCH_SIZE = 1000;
const policyIds = [...policyIdsToFetch];
const results: PackagePolicy[] = [];

for (let i = 0; i < policyIds.length; i += BATCH_SIZE) {
  const batch = policyIds.slice(i, i + BATCH_SIZE);
  const batchResults = await this.server.fleet.packagePolicyService.getByIDs(
    soClient, 
    batch, 
    { ignoreMissing: true }
  );
  results.push(...batchResults);
}

---

Related Issues

• [Synthetics] Silent error handling in package policy deletion can mask failures #254278 - Silent error handling in deletePolicyBulk
• [Synthetics] Race condition in parallel policy operations and unused create response #254279 - Race condition in parallel policy operations and unused create response

Acceptance Criteria

• Remove or document unused _spaceId parameter
• Add constant and/or truncation warning for space aggregation limit
• Improve error handling in getAllSpacesWithMonitors to avoid silent failures
• Optimize getExistingPolicies to reduce unnecessary ID generation

[elasticmachine]

Pinging @elastic/actionable-obs-team (Team:actionable-obs)