-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[chore][graph] Remodel node id as attribute sets #11344
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11344 +/- ##
==========================================
+ Coverage 91.59% 91.60% +0.01%
==========================================
Files 449 449
Lines 23761 23802 +41
==========================================
+ Hits 21763 21804 +41
Misses 1623 1623
Partials 375 375 ☔ View full report in Codecov by Sentry. |
f3779d2
to
b299e68
Compare
d251016
to
f05591b
Compare
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 change generally looks ok to me and aligns with what we've previously discussed around attributes. Probably needs to wait until the RFC for telemetry is merged before moving forward.
`connector "nop/conn" (traces to traces) -> ` + | ||
`processor "nop" in pipeline "traces/1" -> ` + | ||
`connector "nop/conn1" (traces to traces)`, | ||
`connector "nop/conn1" (traces to traces) -> ` + |
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.
Why the change in test here?
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 really just a change in where the start of the cycle is reported, but it still reports the same cycle deterministically.
What it comes down to is that this PR changes the way node IDs are generated. Previously, we built a particualr string for each kind of component. Now we build the ID from the attribute set. Both methods are deterministic, but the graph presumably contains some logic where the node ids are considered in an order that is sensitive to their values (e.g. sort the ids, then for each one, follow the edges, etc). The logical graph that is produced is the same, but when a cycle is encountered, the order in which it is reported is affected by the order in which nodes were considered.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
f05591b
to
f130261
Compare
f130261
to
e2737b4
Compare
Now that #11406 is merged, this should be ready for another review. Important note - this doesn't actually change anything meaningful for users yet. It's just a way of managing the nodes in the graph internally that is closer to what the RFC states should be presented to users. Further work will be needed before users see any standardization of attributes. |
I've converted this back to a draft until a decision is made on #11814. I believe the attribute sets used in this PR suffer from the same problem described in the loggers and that we should solve that problem before this change is meaningful. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR formalizes the notion that attributes are the defining identifiers for component instances in the graph package. It moves us towards the proposal described in #11343.