Add Application Insights telemetry collection specification document #209
Conversation
Ritvik-Jayaswal
requested review from
alaye-ms,
hossain-rayhan and
xgerman
as code owners
docs/designs/appinsights-design.md
Outdated
| - **Properties**: | ||
| - `operator_version`: Semantic version of the operator | ||
| - `kubernetes_version`: K8s cluster version | ||
| - `cloud_provider`: Detected environment (`aks`, `eks`, `gke`, `unknown`) |
There was a problem hiding this comment.
iopenshift, rancher, vmware, etc. as well
docs/designs/appinsights-design.md
Outdated
|
|
||
| ### Operator Health Checks | ||
| - **Metric**: `operator.health.status` | ||
| - **Value**: `1` (healthy) or `0` (unhealthy) |
There was a problem hiding this comment.
what does this mean> Unhealthy operator - how can we act on this?
docs/designs/appinsights-design.md
Outdated
| - **Metric**: `operator.health.status` | ||
| - **Value**: `1` (healthy) or `0` (unhealthy) | ||
| - **Frequency**: Every 60 seconds | ||
| - **Dimensions**: `pod_name`, `namespace` |
There was a problem hiding this comment.
PII better not to collect
docs/designs/appinsights-design.md
Outdated
| ### Cluster Size Metrics | ||
| - **Metric**: `documentdb.cluster.configuration` | ||
| - **Properties per cluster**: | ||
| - `cluster_name`: Name of the DocumentDB cluster |
docs/designs/appinsights-design.md
Outdated
| - **Metric**: `documentdb.cluster.configuration` | ||
| - **Properties per cluster**: | ||
| - `cluster_name`: Name of the DocumentDB cluster | ||
| - `namespace`: Kubernetes namespace |
docs/designs/appinsights-design.md
Outdated
| - **Metric**: `documentdb.replication.lag.bytes` | ||
| - **Description**: Replication lag in bytes | ||
| - **Dimensions**: `cluster_name`, `replica_cluster`, `namespace` | ||
| - **Frequency**: Every 30 seconds |
There was a problem hiding this comment.
let's do a tuple (min, max, avg) over a couple of hours
docs/designs/appinsights-design.md
Outdated
|
|
||
| ### Cluster Environment | ||
| - **Properties** (collected once at startup, attached to all events): | ||
| - `kubernetes_distribution`: `aks`, `eks`, `gke`, `openshift`, `other` |
docs/designs/appinsights-design.md
Outdated
|
|
||
| ## Data Privacy & Security | ||
|
|
||
| - **No PII**: Do not collect usernames, passwords, connection strings, or IP addresses |
There was a problem hiding this comment.
clustername, namespace, storage class can also conatin PII - user would assume they are private.
docs/designs/appinsights-design.md
Outdated
| 3. **Cardinality**: Monitor dimension cardinality to avoid explosion | ||
| 4. **Retry logic**: Implement exponential backoff for telemetry submission failures | ||
| 5. **Local buffering**: Buffer events locally if Application Insights is unreachable | ||
| 6. **Health endpoint**: Expose `/metrics` endpoint for Prometheus scraping |
There was a problem hiding this comment.
why would application monitoring need a promethesu endpoint?
docs/designs/appinsights-design.md
Outdated
|
|
||
| - **No PII**: Do not collect usernames, passwords, connection strings, or IP addresses | ||
| - **Sanitize errors**: Remove sensitive data from error messages | ||
| - **Cluster names**: Use hashed cluster names if privacy required |
There was a problem hiding this comment.
we need to use GUID for that we generate so we can correlate events properly
docs/designs/appinsights-design.md
Outdated
| - **Properties** (collected once at startup, attached to all events): | ||
| - `kubernetes_distribution`: `aks`, `eks`, `gke`, `openshift`, `other` | ||
| - `kubernetes_version`: K8s version | ||
| - `region`: Cloud region (if detectable) |
There was a problem hiding this comment.
You might be able to pull that from the pod.metadata.label["topology.kubernetes.io/region"]
Ritvik-Jayaswal
force-pushed
the
patch-1
branch
from
7cdc39b to
0165f5c
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive telemetry specification document for the DocumentDB Kubernetes Operator, defining Application Insights metrics and events to be collected for operational monitoring, usage analysis, and error tracking.
Changes:
- Added detailed specification document covering 10 categories of telemetry: operator lifecycle, cluster management, lifecycle operations, backup/restore, failover/HA, error tracking, feature usage, performance, compliance, and deployment context
- Defined data privacy and security guidelines including PII protection through GUID-based identifiers and namespace hashing
- Included implementation notes for sampling, batching, cardinality management, and retry logic
| - `resource_id`: Auto-generated GUID of the resource | ||
| - `namespace_hash`: SHA-256 hash of the Kubernetes namespace | ||
| - `error_type`: `cluster-creation`, `backup-failure`, `restore-failure`, `volume-snapshot`, `replication-config`, `tls-cert` | ||
| - `error_message`: Sanitized error message (no PII) |
There was a problem hiding this comment.
The specification states that error messages should be "Sanitized" with "no PII" (line 186), but doesn't provide clear guidance on how to sanitize error messages or what constitutes PII in error contexts. This ambiguity could lead to inconsistent implementations. Consider adding specific examples of sanitization patterns, such as removing resource names, IP addresses, file paths, or user identifiers, and possibly referencing the error categorization approach used elsewhere (line 205).
| - `error_message`: Sanitized error message (no PII) | |
| - `error_message`: Sanitized error message (no PII). The message MUST: | |
| - avoid including raw Kubernetes resource names, namespaces, node names, IP addresses, hostnames, file paths, usernames, email addresses, cloud account IDs, or any token/secret values | |
| - be derived from a stable error category and high-level description (for example, "PVC provisioning failed" or "TLS certificate validation error") rather than raw provider/library error strings | |
| - be safe to log in multi-tenant environments | |
| - when in doubt, prefer mapping to a coarse-grained description based on `error_type` and `error_code` |
| - `namespace_hash`: SHA-256 hash of the Kubernetes namespace | ||
| - `restore_duration_seconds`: Time to restore from backup | ||
| - `backup_age_hours`: Age of backup at restore time | ||
| - `restore_phase`: `starting`, `running`, `completed`, `failed` |
There was a problem hiding this comment.
The specification lists backup_phase values as starting, running, completed, failed, skipped (line 112), and restore_phase values as starting, running, completed, failed (line 145). The restore phase is missing the skipped state. For consistency and completeness, clarify whether restore operations can be skipped, and if so, include this state in the restore_phase values.
| - `restore_phase`: `starting`, `running`, `completed`, `failed` | |
| - `restore_phase`: `starting`, `running`, `completed`, `failed`, `skipped` |
| - `old_primary_index`: Index of the previous primary instance (e.g., 0, 1, 2) | ||
| - `new_primary_index`: Index of the new primary instance |
There was a problem hiding this comment.
The specification includes both "old_primary_index" and "new_primary_index" properties with example values "e.g., 0, 1, 2" suggesting zero-based indexing. However, this conflicts with the DocumentDB cluster architecture where instances_per_node ranges from 1-3 (line 44). Clarify whether these indices are zero-based (0-2 for 3 instances) or one-based (1-3), and ensure consistency with how instances are identified elsewhere in the system.
| - `old_primary_index`: Index of the previous primary instance (e.g., 0, 1, 2) | |
| - `new_primary_index`: Index of the new primary instance | |
| - `old_primary_index`: Zero-based index (instance ordinal) of the previous primary instance (`0..instances_per_node-1`, e.g., `0, 1, 2` for 3 instances) | |
| - `new_primary_index`: Zero-based index (instance ordinal) of the new primary instance (`0..instances_per_node-1`, e.g., `0, 1, 2` for 3 instances) |
| - **Metric**: `documentdb.replication.lag.bytes` | ||
| - **Description**: Replication lag in bytes (aggregated over 2-hour windows) | ||
| - **Dimensions**: `cluster_id`, `replica_cluster_id`, `namespace_hash` | ||
| - **Statistics**: min, max, avg (reported as tuple) | ||
| - **Frequency**: Every 2 hours (aggregated) |
There was a problem hiding this comment.
The metric documentdb.replication.lag.bytes specifies "aggregated over 2-hour windows" with statistics (min, max, avg) reported every 2 hours. This aggregation period seems quite long for a critical replication health metric. High replication lag can indicate serious issues, and a 2-hour reporting delay could mask problems. Consider documenting the rationale for this long aggregation window, or whether a shorter interval (e.g., 5-15 minutes) would be more appropriate for operational monitoring.
| - **Properties**: | ||
| - `sidecar_injector_plugin_enabled`: Boolean indicating if plugin is used | ||
| - `wal_replica_plugin_enabled`: Boolean indicating if plugin is used |
There was a problem hiding this comment.
The specification includes both wal_replica_plugin_enabled (line 230) as a telemetry property, but this plugin is not mentioned anywhere else in the specification. Unlike the sidecar_injector_plugin which is referenced in cluster creation events (line 77), there's no context for what the WAL replica plugin does or when it's relevant. Consider adding documentation about this plugin's purpose and usage context, or if it's not yet implemented, note it as a future feature.
| - **Properties**: | |
| - `sidecar_injector_plugin_enabled`: Boolean indicating if plugin is used | |
| - `wal_replica_plugin_enabled`: Boolean indicating if plugin is used | |
| - **Description**: Tracks usage of optional operator plugins that extend core functionality. | |
| - **Properties**: | |
| - `sidecar_injector_plugin_enabled`: Boolean indicating whether the sidecar injector plugin is enabled for the operator (e.g., for injecting supporting sidecars into DocumentDB pods). | |
| - `wal_replica_plugin_enabled`: Boolean indicating whether the WAL replica plugin is enabled. This is reserved for a future/experimental plugin that manages write-ahead-log (WAL) replication behavior; in operator versions where this plugin is not implemented, this flag MUST remain `false`. |
| - **Dimensions**: | ||
| - `namespace_hash`: SHA-256 hash of the Kubernetes namespace | ||
| - `cloud_provider`: `aks`, `eks`, `gke` | ||
| - `environment`: `aks`, `eks`, `gke` (from spec.environment) |
There was a problem hiding this comment.
The dimension name environment duplicates the cloud_provider dimension on line 35. According to line 36, "environment" comes from "spec.environment", but line 35 already captures cloud provider information. This creates ambiguity - if both dimensions capture the same values (aks, eks, gke), one should be removed or their distinction should be clarified. If they serve different purposes, the specification should explain what "spec.environment" represents and how it differs from the detected cloud_provider.
| - `environment`: `aks`, `eks`, `gke` (from spec.environment) | |
| - `environment`: Logical deployment environment (e.g., `dev`, `staging`, `prod`) from `spec.environment`, distinct from `cloud_provider` |
| - **Value**: `1` (enabled) or `0` (disabled) | ||
| - **Properties**: | ||
| - `cluster_id`: Auto-generated GUID for the DocumentDB cluster | ||
| - `cross_cloud_networking_strategy`: `AzureFleet`, `Istio`, `None` |
There was a problem hiding this comment.
The metric name uses "cross_cloud_networking_strategy" which includes "AzureFleet" as a value, but line 59 refers to it as "participating_cluster_count" suggesting multi-cluster coordination. However, "AzureFleet" is Azure-specific terminology. Consider whether "cross_cloud_networking_strategy" is the appropriate name when one of the values is cloud-specific, or if this should be named more generically (e.g., "multi_cluster_networking_strategy").
| - `cross_cloud_networking_strategy`: `AzureFleet`, `Istio`, `None` | |
| - `multi_cluster_networking_strategy`: `AzureFleet`, `Istio`, `None` |
| - **Properties per cluster**: | ||
| - `cluster_id`: Auto-generated GUID for the DocumentDB cluster (for correlation without PII) | ||
| - `namespace_hash`: SHA-256 hash of the Kubernetes namespace | ||
| - `node_count`: Number of nodes (currently always 1) |
There was a problem hiding this comment.
The specification states "Number of nodes (currently always 1)" which suggests this is a hard-coded value in the current implementation. Recording a telemetry metric that is always 1 provides no analytical value and wastes storage. Consider either removing this property until multi-node support is implemented, or clarifying the purpose of collecting this constant value.
| - `node_count`: Number of nodes (currently always 1) | |
| - `node_count` (optional): Number of nodes in the cluster; omit this property while the operator only supports a single node |
| - `retention_days`: Configured retention period | ||
| - `backup_phase`: `starting`, `running`, `completed`, `failed`, `skipped` | ||
| - `cloud_provider`: Environment where backup was taken | ||
| - `is_primary_cluster`: Boolean indicating if backup from primary |
There was a problem hiding this comment.
The property is_primary_cluster (line 114) is inconsistent with the naming pattern used elsewhere. Other boolean indicators in the specification use verb-based naming like tls_enabled (line 75), server_tls_enabled (line 217), client_tls_enabled (line 218), or sidecar_injector_plugin_enabled (line 229). For consistency, consider renaming this to primary_cluster_backup or from_primary_cluster.
| - `is_primary_cluster`: Boolean indicating if backup from primary | |
| - `from_primary_cluster`: Boolean indicating if backup was taken from primary cluster |
| - `resource_type`: `DocumentDB`, `Backup`, `ScheduledBackup` | ||
| - `resource_id`: Auto-generated GUID of the resource | ||
| - `namespace_hash`: SHA-256 hash of the Kubernetes namespace | ||
| - `error_type`: `cluster-creation`, `backup-failure`, `restore-failure`, `volume-snapshot`, `replication-config`, `tls-cert` |
There was a problem hiding this comment.
The error type list includes tls-cert but there is no corresponding section documenting TLS certificate-related errors or their handling. Given that TLS is a critical security feature (tracked in lines 75, 212-219), certificate errors deserve explicit documentation. Consider adding details about what types of TLS certificate errors will be tracked (e.g., expiration, validation failures, missing certificates).
4 participants
This document outlines the telemetry data points collected by Application Insights for the DocumentDB Kubernetes Operator, detailing metrics for operator lifecycle, cluster management, lifecycle operations, backup and restore, failover, error tracking, feature usage, performance, compliance, and deployment context.