[Logs UI] Shared <LogStream /> component
#76262
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afgomez
force-pushed
the
75650-log-stream-shared
branch
from
0f9e71c to
11e34ab
Compare
afgomez
changed the title
<LogStream /> component log stream shared<LogStream /> component
The `<LogStream />` component misbehaves when its parent component doesn't have a fixed width. This is caused by an `AutoSizer` trying to determine the necessary width, and the `ScrollableLogTextStreamView` trying to hide the scrollbar by making the scrollable view 20 pixels wider than its container. To fix this we control the hiding behaviour through a prop, and disable it in our component.
afgomez
force-pushed
the
75650-log-stream-shared
branch
from
1fc8be9 to
690130b
Compare
afgomez
added
Feature:Logs UI
afgomez
added
release_note:skip
|
I was playing with
|
|
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
jasonrhodes
reviewed
Sep 1, 2020
|
Pinging @elastic/apm-ui (Team:apm) |
|
Wow, this is awesome @afgomez ! |
sorenlouv
reviewed
Sep 1, 2020
| startTimestamp={Date.now() - 86400000} | ||
| endTimestamp={Date.now()} | ||
| query={`trace.id: "${urlParams.traceId}" OR "${urlParams.traceId}"`} | ||
| /> |
There was a problem hiding this comment.
This interface lgtm 👍 Thanks for doing this
This has been fixed.
We discussed this somewhere else but I'll add my opinion here for completion. I consider this shared component library code (vs application code). I think when something exceptional happens the component should throw an exception and let the consuming application code decide what to do. Maybe they want to recover from it, maybe they want to crash and burn the app because there's nothing to do if this component didn't load, but I don't think library code should make that decision. |
Makes sense. It's hard to imagine a case where this component would be on its own somewhere else, but you're right that we should probably let the caller make the decision. Can we add a casual recommendation to consider wrapping the LogStream component in an |
weltenwort
reviewed
Sep 3, 2020
|
|
||
| To use the component, there are several things you need to ensure in your plugin: | ||
|
|
||
| - In your `kibana.json` plugin, you need to either add `"requiredBundles": ["infra"]` or `"requiredPlugins": ["infra"]`. |
There was a problem hiding this comment.
Would requiredBundles be enough given that this component uses the infra HTTP API to fetch the data?
There was a problem hiding this comment.
Mmm, good point. I haven't tried what would happen if the infra plugin is disabled.
There was a problem hiding this comment.
Maybe we can provide a "is the logs api available?" wrapper alongside that renders an informative message instead of the children? (In a separate PR, though.)
There was a problem hiding this comment.
I checked what happens with xpack.infra.enabled: false. The component renders, but it says there's no log data. I will remove the requiredBundles bit from the docs because it's a lie :D.
There was a problem hiding this comment.
I think I want to give freedom to plugin developers here. They can either choose to make it a hard requirement (requiredPlugins), or a soft one (optionalPlugins). In the second case, we can advise to wrap our component in a check that tests if the infra plugin is loaded or not, alongside other changes that they want to do in their UI.
function SomeComponent() {
const { services } = useKibana();
const hasInfraPlugin = 'infra' in services; // This seems to work
const tabs = ['Timeline', 'Metadata'];
if (hasInfraPlugin) {
tabs.push('Logs');
}
// ...
}We can give examples in the docs. Do you think this is enough, or do you think it's the responsibility of our component to check for this? We can of course also check on our side and raise an exception.
Edit: It's not possible. It needs to be in requiredPlugins if the component is imported
There was a problem hiding this comment.
I think if it's in optional plugins, then they need to add it to requiredBundles as I understand it.
|
This is awesome!
|
jasonrhodes
approved these changes
Sep 4, 2020
There was a problem hiding this comment.
I finally got this working locally and it looks good. I wonder a couple things as we iterate on this shared component:
- This separated from the context of the Logs UI, I'm not sure if the chosen columns for the source configuration are a great idea. Maybe this should just be timestamp and message by default?
- It also feels a bit strange that I can't interact with the message to learn more about it at all. I think clicking on the log line should take you to the Logs UI but at the very least some kind of link to the Logs UI somewhere may be good, either clicking the log line, using the actions popover menu, or we let APM and other users of this shared plugin handle this on their own, e.g. one single "view in Logs UI" link at the top of the tab content here
But as a first step this looks great and we should get it merged so others can weigh in with feedback.
💛 Build succeeded, but was flaky
Test FailuresAccessibility Tests.test/accessibility/apps/discover·ts.Discover Load a new search from the panelStandard OutStack TraceBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
|
Thanks for the review @jasonrhodes! I'll answer to your feedback here
I think for consistency the default should be the same as in the logs UI. I do think that there's value in letting users specify what columns they want, but is not that easy so we decided to postpone it for now.
I think it makes sense to allow consumers to specify actions, but I'm not sure how I feel about adding default actions. For the link to the logs UI, I would prefer to have it separate. That way consumers can decide where in their UI makes sense to link to the logs UI and how (maybe with a extended time range, different filters, etc). We can export a helper to generate the URLs or a "View in logs" component to streamline the process. |
afgomez
pushed a commit
that referenced
this pull request
Sep 8, 2020
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Sep 8, 2020
…' of github.com:jloleysens/kibana into ilm/fix/pre-existing-policy-with-no-existing-repository * 'ilm/fix/pre-existing-policy-with-no-existing-repository' of github.com:jloleysens/kibana: fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751)
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Sep 8, 2020
…s-for-710 * 'master' of github.com:elastic/kibana: [Search] Use async es client endpoints (elastic#76872) fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751) # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx # x-pack/plugins/index_lifecycle_management/common/types/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/api.ts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #75650
This PR implements an embeddable
<LogStream />component that other plugins can use.For the @elastic/apm-ui folks, the code I added to your area is a proof of concept and it's not intended to be merged in this PR. Feel free to play with it though :)
To Do
Non-goals for this PR
Checklist
Delete any items that are not applicable to this PR.