-
Notifications
You must be signed in to change notification settings - Fork 569
ref: Make logs, metrics go via scope #5213
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: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5213 +/- ##
==========================================
- Coverage 84.25% 84.21% -0.05%
==========================================
Files 181 181
Lines 18463 18469 +6
Branches 3288 3283 -5
==========================================
- Hits 15556 15553 -3
- Misses 1894 1900 +6
- Partials 1013 1016 +3
|
|
|
||
| def _capture_log(self, log: "Optional[Log]") -> None: | ||
| if not has_logs_enabled(self.options) or log is None: | ||
| def _capture_telemetry( |
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 now essentially a dispatcher on ty. Unfortunately we need the explicit type and can't look at the actual type of the thing coming in since it's all just dicts. I'll be exploring whether we can make this better without introducing breaking changes.
Description
Logs and metrics were going through a completely separate pipeline compared to other events.
Conceptually, they're still different from regular events since they're more lightweight and attribute-based (no data, contexts, etc., everything is an attribute) so separate handling makes sense. However, the pipeline should still conceptually resemble the one we use for other event types, for consistency.
Current pipeline for non-log, non-metric events
scope.capture_XXX. This merges the current scope stack and callsclient.capture_XXXwith the resulting merged scope.client.capture_XXXcontains virtually all of the logic, most notably:scope.apply_to_event, populating contexts, user data, etc.This PR
client.capture_XXX, we callscope.capture_XXX, like we do for other event types, and then callclient.capture_XXXfrom there.client.capture_XXXnow calls a newscope.apply_to_telemetryfunction internally (akin toscope.apply_to_event, but sets attributes instead).Additionally:
format_attributeto utilsNote: This is just a first PR, I will refactor further.
Issues
Reminders
tox -e linters.feat:,fix:,ref:,meta:)