Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates OpenTelemetry integration behavior in the Hyperf package, apparently to reduce exporter throttling, by adjusting URI sanitization and changing which listeners are registered by default.
Changes:
- Expanded UUID URI masking to match additional UUID versions.
- Removed periodic metrics/trace flush listeners from the default listener registration, leaving shutdown + DB query listener(s).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Support/Uri.php |
Broadens UUID matching used by Uri::sanitize() when masking path segments. |
src/ConfigProvider.php |
Stops registering periodic trace/metric flush listeners by default, altering flush/export behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static function sanitize(string $uri, array $uriMask = []): string | ||
| { | ||
| $uuid = '/\/(?<=\/)([0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})(?=\/|$)/i'; | ||
| $uuid = '/\/(?<=\/)([0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})(?=\/|$)/i'; |
There was a problem hiding this comment.
The UUID masking regex was broadened to accept versions 6–8. There are existing unit tests for Uri::sanitize() UUID masking, but none covering the newly supported UUID versions—please add at least one test case (e.g., a v7 UUID) to lock in the new behavior and prevent regressions.
| 'listeners' => [ | ||
| Listener\MetricFlushListener::class, | ||
| Listener\TraceFlushListener::class, | ||
| Listener\OtelShutdownListener::class, |
There was a problem hiding this comment.
Removing MetricFlushListener / TraceFlushListener from the default listeners list changes the library’s runtime behavior: metrics/traces will no longer be force-flushed on a periodic timer, so data may be delayed until SDK-internal scheduling or worker shutdown. If this is meant to address exporter throttling, consider making this behavior configurable (or updating the published config/docs) since open-telemetry.{traces,metrics}.export_interval will no longer have an effect by default.
| Listener\OtelShutdownListener::class, | |
| Listener\OtelShutdownListener::class, | |
| Listener\TraceFlushListener::class, | |
| Listener\MetricFlushListener::class, |
| 'listeners' => [ | ||
| Listener\MetricFlushListener::class, | ||
| Listener\TraceFlushListener::class, | ||
| Listener\OtelShutdownListener::class, | ||
| Listener\DbQueryExecutedListener::class, |
There was a problem hiding this comment.
PR description currently contains only the template and doesn’t explain how these changes (UUID sanitization + removing periodic flush listeners) relate to “Fix Throttling” or whether this is a breaking/default-behavior change. Please update the PR description (and/or changelog) with the throttling root cause and the intended new flush/export behavior so users can migrate safely.
Description
Explain what this PR does and why it is needed.
Type of change
Checklist
feat: add SQS aspect)composer testpasses locallyRelated issues
Closes #, Relates to #