-
Notifications
You must be signed in to change notification settings - Fork 580
feat(span-streaming): Add spans to telemetry pipeline, add span name and attributes (3) #5399
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
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Span Streaming
Bug Fixes 🐛
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
Codecov Results 📊✅ 13 passed | Total: 13 | Pass Rate: 100% | Execution Time: 11.79s All tests are passing successfully. ❌ Patch coverage is 21.57%. Project has 13716 uncovered lines. Files with missing lines (180)
Generated by Codecov Action |
| self, telemetry: "Union[Log, Metric]" | ||
| self, telemetry: "Union[Log, Metric, StreamedSpan]" | ||
| ) -> None: | ||
| # TODO: turn Logs, Metrics into actual classes |
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.
Want to do this eventually, but it's a breaking change so it has to wait.
| span_id = trace_context.get("span_id") | ||
| if telemetry.get("span_id") is None and span_id: | ||
| telemetry["span_id"] = span_id | ||
| if not isinstance(telemetry, StreamedSpan): |
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 stuff for metrics and logs in this if should be moved elsewhere, but out of scope of this PR.
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| trace_id: "Optional[str]" = None, | ||
| ): | ||
| self.name: str = name | ||
| self._attributes: "Attributes" = attributes or {} |
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.
Initial attributes in StreamedSpan not formatted
Medium Severity
StreamedSpan.__init__ assigns attributes directly without calling format_attribute on each value, unlike the existing patterns in logger.py and metrics.py. This creates inconsistent behavior between using __init__(attributes=...) versus set_attribute()/set_attributes(), and violates the design principle stated in format_attribute's docstring: "We do this as soon as a user-provided attribute is set, to prevent spans, logs, metrics and similar from having live references to various objects." Unformatted attributes could retain live object references that mutate before serialization.
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.
This is actually correct, will fix
|
|
||
| def _capture_span(self, span: "StreamedSpan", scope: "Scope") -> None: | ||
| pass | ||
|
|
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.
Missing span_batcher in uWSGI thread support check
Low Severity
span_batcher spawns a background flusher thread via the inherited _ensure_thread method, but the condition checking whether to call check_uwsgi_thread_support() only includes self.log_batcher, not self.span_batcher. When running under uWSGI with threading disabled and only span streaming enabled (no logs, profiling, or monitor), users won't receive the warning about thread support issues, and spans will be silently dropped when thread creation fails.
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.
Will fix in follow up PR


StreamedSpansince they're used in the pipelineChipping away at #5317 to transform it into reviewable and mergeable PRs.