-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ObsUX][APM] Rename observability setting apmEnableMultiSignal
to entityCentricExperience
#188097
Conversation
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
🤖 GitHub commentsExpand to view the GitHub comments
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.
On the documentation side, please take a look at our config docs and make sure there are no changes or entries needed. Thanks! Approving to unblock.
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 seems like users who have previously opted into apmEnableMultiSignal will need to re-opt into the new setting. Is that intended?
My understanding is that we want to replace this flag and as this is a new feature it should be fine to have the new flag instead (and ignore the old one). @roshan-elastic is that correct? I think it will make sense to backport it to 8.15 as this flag has not been released yet and we can have it there with the new name directly |
Yeah @jennypavlova - that's right. The existing feature flag was off by default and it's unlikely anyone ever found it (it was only in stack management). Even if they turned it on, it wouldn't have really done anything. When I tested, it basically just made APM blank (presumably it was work in progress still). |
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
Changing it isn't a problem like Roshan said, but it recommended to migrate them. I don't think it's necessary 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.
LGTM!
There are still a few things besides this setting named "multi signal". Are we planning on renaming those too?
@crespocarlos As mentioned here we are renaming it because we will include multiple experiences under the same feature flag, IMO multi signal is still valid for the component names, please let me know if you think otherwise. |
Yeah. multi signal makes sense as you said. Thanks! |
@bmorelli25 The feature flag was not there before I renamed it so I can confirm that I didn't change anything there. I guess we don't need any changes there in this case, right? |
defaultMessage: | ||
'{technicalPreviewLabel} Enable the multi-signal feature in APM, which allows you to monitor services from logs and traces.', | ||
description: i18n.translate('xpack.observability.entityCentricExperienceDescription', { | ||
defaultMessage: '{technicalPreviewLabel} Promote entity-centric experience to users.', |
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.
Can we add a better description here? It doesn't say much more than the title itself. Maybe @mdbirnstiehl could help?!
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.
cc: @roshan-elastic
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…entityCentricExperience` (elastic#188097) Closes elastic/observability-dev#3731 ## Summary This PR rename `apmEnableMultiSignal` to `entityCentricExperience` <img width="1834" alt="image" src="https://github.com/elastic/kibana/assets/14139027/bc8dfcf6-f739-4215-9f68-47345fbdef5e"> Co-authored-by: Katerina <[email protected]> (cherry picked from commit 9291a4f) # Conflicts: # packages/kbn-management/settings/setting_ids/index.ts
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
1 similar comment
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11114971159 |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11121442959 |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
Based on this comment, I'll switch this PR to |
Closes https://github.com/elastic/observability-dev/issues/3731
Summary
This PR rename
apmEnableMultiSignal
toentityCentricExperience