adding apache, fpm, k8s resource detectors #444
Conversation
this ports the detectors from open-telemetry/opentelemetry-php#1628 into contrib
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
============================================
- Coverage 82.85% 82.39% -0.46%
- Complexity 2353 2508 +155
============================================
Files 164 167 +3
Lines 9563 9897 +334
============================================
+ Hits 7923 8155 +232
- Misses 1640 1742 +102 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| $components = [ | ||
| 'k8s', | ||
| $podUid, | ||
| $this->getPodName() ?? 'unknown-pod', | ||
| $this->getNamespace() ?? 'default', | ||
| ]; |
There was a problem hiding this comment.
While the following spec note is about the Collector, it seems to apply here too - the code currently generates the same instance id for all containers in a pod:
https://opentelemetry.io/docs/specs/semconv/registry/attributes/service/#service-instance-id
It’s not recommended for a Collector to set
service.instance.idif it can’t unambiguously determine the service instance that is generating that telemetry. For instance, creating an UUID based onpod.namewill likely be wrong, as the Collector might not know from which container within that pod the telemetry originated.
| ]; | ||
|
|
||
| // Create a stable UUID v5 using a namespace UUID and deterministic name | ||
| $namespace = Uuid::fromString('4d63009a-8d0f-11ee-aad7-4c796ed8e320'); // DNS namespace UUID |
There was a problem hiding this comment.
| $namespace = Uuid::fromString('4d63009a-8d0f-11ee-aad7-4c796ed8e320'); // DNS namespace UUID | |
| $namespace = Uuid::fromString('4d63009a-8d0f-11ee-aad7-4c796ed8e320'); |
Same comment needs to be removed in the kubernetes detector.
| * For Apache mod_php environments, generates a stable instance ID based on the server name and hostname | ||
| * rather than using random UUIDs which cause cardinality explosion in metrics. |
There was a problem hiding this comment.
This seems like it will violate the single writer principle when not setting process.pid / when using threads instead of processes?
https://opentelemetry.io/docs/specs/otel/metrics/data-model/#single-writer
Aggregations of metric streams MUST only be written from a single logical source at any given point time.
For example, if an SDK implementation fails to find uniquely identifying Resource attributes for a component, then all instances of that component could be reporting metrics as if they are from the same resource. In this case, metrics will be reported at inconsistent time intervals. For metrics like cumulative sums, this could cause issues where pairs of points appear to reset the cumulative sum leading to unusable metrics.
2 participants
this ports the detectors from open-telemetry/opentelemetry-php#1628 into contrib
NB that I haven't yet ported across the sdk-config parts yet, which can be a follow-up.
Replaces: open-telemetry/opentelemetry-php#1628