-
Notifications
You must be signed in to change notification settings - Fork 421
feat: Added reduced type for partial granularity traces.
#3540
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
a24435a to
921c085
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3540 +/- ##
==========================================
+ Coverage 89.73% 90.09% +0.36%
==========================================
Files 426 412 -14
Lines 56229 55242 -987
Branches 1 1
==========================================
- Hits 50457 49771 -686
+ Misses 5772 5471 -301
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbcb533 to
69cd542
Compare
69cd542 to
a4ac9a3
Compare
| /** | ||
| * This keeps a static list of attributes that are used by | ||
| * one or more entity relationship rules to synthesize an entity relationship. | ||
| * Note: These attributes also have corresponding TraceSegment attributes | ||
| * as this list is checked before a span is made. The ones that are TraceSegment | ||
| * attributes are noted in the comments. Any new span attributes being added must | ||
| * be checked to ensure those are what is getting assigned to the TraceSegment as well. | ||
| */ |
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.
I don't quite understand this comment. Are we supposed to remember this list exists and add things to it as we support new features?
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.
For now, yes. I raised this from the beginning of the project and people don't seem to mind keeping hardcoded list of attributes that produce entity relationships.
The mapping of segment attrs to spans will get removed. The Java and python agent are synthesizing all spans first then applying partial granularity logic. Even though that seems inefficient, we might refactor this to be consistent and drop the mapping of segment entity relationship attrs
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.
I think we need a documentation/internals/ doc that walks a developer through adding a new span type or whatever that would need to modify this list.
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.
I'm not sure I follow. We seem to be adding more internal documentation to our repo. A code comment seems enough in this case.
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.
It is unlikely anyone is going to remember this list and comment exists the next time something is added which will need to touch it. I am suggesting we have documentation for adding a feature that calls out updating this. For example:
# Add A New Span Type
Spans represent blah blah stuff... To support a new span type we must:
1. Update file X with some code that looks like: <snippet>
2. Update the `SPAN_ENTITY_RELATIONSHIP_ATTRIBUTES` with new attributes that must be included regardless of sampler settings.
3. Etc
4. EtcMaybe it's not a long document. But I think the whole team would benefit from easy to reference material that clearly shows how to make these rarely made changes.
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.
It's not a span type. It's whenever new entity relationships are added that rely on new span attributes to make the relationship. The current process will be that our teams will be notified and have a story to address by adding new attributes to the list
Description
This PR adds the
reducedpartial granularity logic: keep entry spans, llm spans, and any exit spans that have entity relationship attributes.This is blocked until #3536 is done because this assumes that a sampling decision has been made and the transaction has assigned
.isPartialTraceproperty signaling the span event logic to execute for a partial trace based on the typeRelated Issues
Closes #3455