-
Notifications
You must be signed in to change notification settings - Fork 212
feat: Add Apache and FPM resource detectors with stable service instance IDs #1628
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.
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
base: main
Are you sure you want to change the base?
Conversation
This resolves high-cardinality issues in metrics by providing stable service instance IDs for Apache and FPM environments instead of random UUIDs that change on process restart. Features: - Apache detector: Generates stable IDs using server name + hostname + document root - FPM detector: Generates stable IDs using pool name + hostname - Cascading detection: Service UUID → Runtime-specific stable ID → User override - Proper precedence: OTEL_SERVICE_NAME takes priority over OTEL_RESOURCE_ATTRIBUTES Implementation: - Added detector classes in src/SDK/Resource/Detectors/ - Added component providers for configuration system - Registered detectors in SPI system and known values - Updated ResourceInfoFactory with correct precedence order - Comprehensive test coverage for unit and integration scenarios The detectors only activate in their respective environments (apache2handler/fmp-fcgi SAPI) and return empty resources otherwise, allowing clean fallback behavior.
Note to reviewers: The remaining issues are in unreleated code. About this PR: the issue is that systems that generate metrics from traces usually need an instance label. The uuid per process is an issue here as one uuid per spawned process would be set. we could use apcu to create even longer-running stable ids. |
… issues - Fix webengine.version to contain only version number (e.g. "2.4.41") - Fix webengine.description to contain full version details per spec - Add Apache version number extraction with regex parsing - Add comprehensive test coverage for version extraction - Remove unnecessary PHPStan ignore annotations in protobuf code - Add GitHub Actions concurrency control to prevent redundant CI runs The Apache detector now properly separates version number from description following OpenTelemetry semantic conventions, similar to the spec example where webengine.version="21.0.0" and webengine.description contains full details.
Updates Apache and FPM resource detectors to generate proper UUID v5 values for service.instance.id instead of custom hash-based identifiers, ensuring full compliance with OpenTelemetry semantic conventions. Changes: - Replace hash-based IDs with deterministic UUID v5 generation - Use DNS namespace UUID (RFC 4122) for consistent namespace - Maintain stable IDs across process restarts for same environment - Update tests to verify proper UUID format compliance - Clean up unused hash imports and improve code style The detectors now generate spec-compliant UUIDs like: 550e8400-e29b-41d4-a716-446655440000 This maintains the high-cardinality solution while adhering to the OpenTelemetry specification requirement for UUID-format service instance IDs.
maybe related: #1489 |
definitely related. - this detector should always be included and always be progressively overriden if possible. |
$resourceDetector = new Apache(); | ||
$reflection = new \ReflectionClass($resourceDetector); | ||
$isApacheSapiMethod = $reflection->getMethod('isApacheSapi'); | ||
$isApacheSapiMethod->setAccessible(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the min php version required is 8.1 (see composer.json)
this method has no effect since PHP 8.1: https://www.php.net/manual/en/reflectionmethod.setaccessible.php
- Add KubernetesDetector to detect K8s pod metadata and generate stable instance IDs - Register 'k8s' detector in known values configuration - Add component provider for Kubernetes detector configuration - Include comprehensive test coverage for detector functionality - Prevent high cardinality issues by using pod UID instead of random UUIDs
// Add service name if configured | ||
$serviceName = Configuration::has(Variables::OTEL_SERVICE_NAME) | ||
? Configuration::getString(Variables::OTEL_SERVICE_NAME) | ||
: null; | ||
|
||
if ($serviceName !== null) { | ||
$attributes[ResourceAttributes::SERVICE_NAME] = $serviceName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this should not be part of the detectors. Users can use these detectors together with the service
detector if they want to read OTEL_SERVICE_NAME
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This duplicates the service
detector, which is mandatory since a recent change (so it should always run). In practice, that might also clobber the service.instance.id
that this detector sets? I think we need an integration test for ServiceInfoFactory
, because users generally rely on this to set up resources, and it applies the service
detector last.
* For Kubernetes environments, generates a stable instance ID based on the pod UID | ||
* rather than using random UUIDs which cause cardinality explosion in metrics. | ||
*/ | ||
final class Kubernetes implements ResourceDetectorInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom resource detectors related to generic platforms (e.g. Docker, Kubernetes) or vendor specific environments (e.g. EKS, AKS, GKE) MUST be implemented as packages separate from the SDK.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Any updates on this? This would solve my issue where im using the new file based configuration in a phpfpm production setup. With the file based configuration the "service" resource discovery is mandatory, there is currently no way to override this instance id. |
Co-authored-by: Tobias Bachert <[email protected]>
this ports the detectors from open-telemetry/opentelemetry-php#1628 into contrib
@cedricziel I hope you don't mind, I've ported these detectors over to contrib in open-telemetry/opentelemetry-php-contrib#444 |
This resolves high-cardinality issues in metrics by providing stable service instance IDs for Apache and FPM environments instead of random UUIDs that change on process restart.
Features:
Implementation:
The detectors only activate in their respective environments (apache2handler/fmp-fcgi SAPI) and return empty resources otherwise, allowing clean fallback behavior.