-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Data Set Quality] Hide Data set details when dataStream is coming from remote cluster #220529
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
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
| prefix: qualityIssuesAccordionTitle, | ||
| }); | ||
|
|
||
| const isCCSRemoteIndex = isCCSRemoteIndexName(rawDoc._index ?? ''); |
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.
QQ: Just curious if the _index can actually really be undefined? and do we want to display the Dataset Quality link in this case or not?
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 can answer this from the Discover side, yes it can be for ES|QL mode if METADATA _index isn't used in the query. Although I'm not sure if the logs profile is currently showing the degraded fields section in ES|QL mode yet.
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.
In DatasetQualityLink there is a line that would hide the link when dataStream is empty:
if (!locator || !dataStream) return null;
I think that if _index would be empty, the dataStream should be too ( correct me if I'm wrong)
If somehow dataStream would be present but _index wouldn't, the link itself would still work properly.
|
I've noticed some issues, but they are not related to this PR:
You can check the doc in the release-oblt cluster. Screen.Recording.2025-05-21.at.09.09.47.mov |
Actually they both appear as empty in the |
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
kertal
left a comment
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.
While the code change LGTM, could you provide a bit more context, is this resolving an issue? it's labeled as bug, but this would be more suitable as a fix. And this should also be described in the PR description. Also a unit test would be great to cover it, thx 👍
Hi, you are right. So the context is that now CCS clusters do not work with Dataset Quality (and for now there aren't any plans to change it). When you click that link it would take you to the page and it will result with an error. So for the logs that are coming from remote cluster we want to hide the link to dataset quality in the flyout. I also added two tests to make sure it would work properly |
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
|
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
| const hitWithDataStream = buildHit({}); | ||
|
|
||
| hitWithDataStream.raw.fields = sourceFields; |
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.
q: is it possible to do something like const hitWithDataStream = buildHit(sourceFields);?
|
|
||
| const accordions = screen.getAllByTestId('unifiedDocViewLogsOverviewDegradedFieldsAccordion'); | ||
| const accordion = accordions[accordions.length - 1]; | ||
| const button = accordion.querySelector('button'); |
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 do within(accordion).getByRole('button')?
| const accordion = accordions[accordions.length - 1]; | ||
| const button = accordion.querySelector('button'); | ||
|
|
||
| if (button) { |
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.
is the button not always there or is this just to appease the type checking?
| act(() => { | ||
| button.click(); | ||
| }); |
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 do something like
const user = userEvent.setup();
await user.click(button)
| const remoteHit = buildHit({}); | ||
|
|
||
| remoteHit.raw.fields = sourceFields; | ||
| remoteHit.raw._index = `remoteCluster:${DATA_STREAM_NAME}`; |
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.
same as before, is it possible to do buildHit(sourceFields) and maybe extend that mock factory to also take the index?
AlexGPlay
left a comment
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.
left some comments around testing - is it possible to somehow test this manually?
|
Thank you for the review.
Let me know if anything is unclear! |
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
| const accordions = screen.getAllByTestId('unifiedDocViewLogsOverviewDegradedFieldsAccordion'); | ||
| const accordion = accordions[accordions.length - 1]; | ||
| const button = accordion.querySelector('button'); | ||
|
|
||
| if (button === null) { | ||
| return; | ||
| } | ||
| // Check the aria-expanded property of the button | ||
| const isExpanded = button.getAttribute('aria-expanded'); | ||
| expect(isExpanded).toBe('false'); | ||
| act(() => { | ||
| button?.click(); | ||
| }); |
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 do the same we did for the other 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.
@AlexGPlay I updated all buttons in that test file so now it should be consistent
| const accordions = screen.getAllByTestId('unifiedDocViewLogsOverviewDegradedFieldsAccordion'); | ||
| const accordion = accordions[accordions.length - 1]; | ||
| const button = accordion.querySelector('button'); | ||
|
|
||
| act(() => { | ||
| button?.click(); | ||
| }); |
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.
same here please 🙏
| act(() => { | ||
| button?.click(); | ||
| }); |
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.
| act(() => { | |
| button?.click(); | |
| }); | |
| const user = userEvent.setup(); | |
| await user.click(button); |
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.
Oh you meant those changes 😄
Got it now
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.
yeah, no worries, other than this i think it looks good
| act(() => { | ||
| button?.click(); | ||
| }); |
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.
| act(() => { | |
| button?.click(); | |
| }); | |
| const user = userEvent.setup(); | |
| await user.click(button); |
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.
Right now it should be ok
AlexGPlay
left a comment
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.
thank you for taking your time getting the tests to a better state 😄
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
…vigating-to-the-data-set-details-page-for-a-remote-data-stream
💚 Build Succeeded
Metrics [docs]Async chunks
History
|
|
/oblt-deploy |
yngrdyn
left a comment
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, I tested the changes and the link is hidden when we are checking a log coming from a remote cluster and is shown when the log is local
Screen.Recording.2025-06-11.at.11.18.00.mov
|
Starting backport for target branches: 8.19 |
…om remote cluster (elastic#220529) Because Data Set quality page does not currently work with CCS, logs coming from remote cluster have dataset quality link hidden in the flyout. The fix would hide a Data Set Quality link for documents that are coming from remote clusters Fixes elastic#211602 --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit fa400a4)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ming from remote cluster (#220529) (#223376) # Backport This will backport the following commits from `main` to `8.19`: - [[Data Set Quality] Hide Data set details when dataStream is coming from remote cluster (#220529)](#220529) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Robert Stelmach","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-06-11T09:19:51Z","message":"[Data Set Quality] Hide Data set details when dataStream is coming from remote cluster (#220529)\n\nBecause Data Set quality page does not currently work with CCS, logs\ncoming from remote cluster have dataset quality link hidden in the\nflyout.\n\nThe fix would hide a Data Set Quality link for documents that are coming\nfrom remote clusters\n\nFixes #211602\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"fa400a4ebb984a05f8f3f0b73c8901e33d8877d2","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:obs-ux-logs","Feature:Dataset Health","backport:version","v9.1.0","v8.19.0"],"title":"[Data Set Quality] Hide Data set details when dataStream is coming from remote cluster","number":220529,"url":"https://github.com/elastic/kibana/pull/220529","mergeCommit":{"message":"[Data Set Quality] Hide Data set details when dataStream is coming from remote cluster (#220529)\n\nBecause Data Set quality page does not currently work with CCS, logs\ncoming from remote cluster have dataset quality link hidden in the\nflyout.\n\nThe fix would hide a Data Set Quality link for documents that are coming\nfrom remote clusters\n\nFixes #211602\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"fa400a4ebb984a05f8f3f0b73c8901e33d8877d2"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/220529","number":220529,"mergeCommit":{"message":"[Data Set Quality] Hide Data set details when dataStream is coming from remote cluster (#220529)\n\nBecause Data Set quality page does not currently work with CCS, logs\ncoming from remote cluster have dataset quality link hidden in the\nflyout.\n\nThe fix would hide a Data Set Quality link for documents that are coming\nfrom remote clusters\n\nFixes #211602\n\n---------\n\nCo-authored-by: kibanamachine <[email protected]>","sha":"fa400a4ebb984a05f8f3f0b73c8901e33d8877d2"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Robert Stelmach <[email protected]>
…om remote cluster (elastic#220529) Because Data Set quality page does not currently work with CCS, logs coming from remote cluster have dataset quality link hidden in the flyout. The fix would hide a Data Set Quality link for documents that are coming from remote clusters Fixes elastic#211602 --------- Co-authored-by: kibanamachine <[email protected]>
…om remote cluster (elastic#220529) Because Data Set quality page does not currently work with CCS, logs coming from remote cluster have dataset quality link hidden in the flyout. The fix would hide a Data Set Quality link for documents that are coming from remote clusters Fixes elastic#211602 --------- Co-authored-by: kibanamachine <[email protected]>
Because Data Set quality page does not currently work with CCS, logs coming from remote cluster have dataset quality link hidden in the flyout.
The fix would hide a Data Set Quality link for documents that are coming from remote clusters
Fixes #211602