-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[connector/servicegraph] Update virtual node defaults and change feature gate to beta #31735
[connector/servicegraph] Update virtual node defaults and change feature gate to beta #31735
Conversation
84a2fae
to
024fc6a
Compare
024fc6a
to
69d2ab0
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.
This a breaking change, but since the feature was in alpha I think it's still ok to change the attribute list. It now matches the Tempo defaults, so I'm good with the change. Thanks!
71cb827
to
201df8d
Compare
I have rebased + updated the defaults in the feature gate description 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.
LGTM
@jpkrohling can this be merged? |
@ogxd Sorry for the delay, I think we should add a changelog to notify users |
I doubt that not work. opentelemetry-collector-contrib/connector/servicegraphconnector/connector.go Lines 383 to 394 in 626046f
opentelemetry-collector-contrib/connector/servicegraphconnector/connector.go Lines 633 to 642 in 626046f
Even when the |
…om/ogxd/opentelemetry-collector-contrib into servicegraphconnector-virtualnodes
Indeed, I just added a check to disable the feature if |
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
Please excuse me for the bothering 😅 |
Description:
Link to tracking Issue: #31734
Testing: No functional change requiring additional tests
Documentation: Updated documentation