-
Notifications
You must be signed in to change notification settings - Fork 222
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
Implemented span compression algorithm #1321
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
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.
Did a brief review, everything looks good so far! I'll do a deeper review once this is ready for review.
I noticed that you haven't really refactored leaf
to exit
-- is that still something we want to do? I made a note here after one of our previous discussions.
def report(self) -> None: | ||
self.tracer.queue_func(SPAN, self.to_dict()) | ||
|
||
def try_to_compress(self, sibling: SpanType) -> bool: |
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.
Maybe some docstrings through here so we can remember at a glance what composite vs regular means?
I started it implementing in this branch, but soon realized that it would overload it (something that also became apparent with introducing type hints with this PR, hence I only committed the ones in |
This cleans up a wart where `traceparent` is initialized to None, and needs to be set by the callee right after, as there is code that assumes `traceparent` to be set.
we no longer only call `child_ended` for breakdown metrics
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.
LGTM! 👍
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.
Docs look good!
this was initially part of elastic#1321
this was initially part of #1321
# Conflicts: # elasticapm/traces.py
What does this pull request do?
See elastic/apm#432
Related issues
closes #1152