Skip to content
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

Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards #18641

Merged
merged 30 commits into from
Aug 26, 2019

Conversation

hugohaggmark
Copy link
Contributor

What this PR does / why we need it:
This PR adds the ability to create a Logs Panel visualization for a Dashboard.

Which issue(s) this PR fixes:
Closes #14576

Special notes for your reviewer:
Remember to test logs in Explore as well as Logs Panel and Explore share components.

@hugohaggmark hugohaggmark self-assigned this Aug 20, 2019

if (this.replacePreviousResults) {
return sortedNewResults;
const slice = 1000;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a bug, we didn't reduce the rows when replacing previous results which could potentially lead to lot's of rows

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This limit should be datasource-dependent.

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@dprokop @ryantxu will need some help reviewing this, and @davkal if you can do some testing of both explore logs & dashboard logs and look out for regression that great.

Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great Hugo -- loads more stuff in @grafana/ui! I made a few minor comments.

I tried it in dashboard and it works as expected. Obviously more we can do... but this is a great first step.

I have not tried it in explore and don't know what edge cases to try there ;)

great work

@hugohaggmark
Copy link
Contributor Author

@dprokop I added the label add to changelog grafana/ui as well, is that ok?

Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a solid thing @hugohaggmark, looking good :) Left some minor comments

}

interface State {
hiddenSeries: string[];
showAllTimeSeries: boolean;
}

export class ExploreGraphPanel extends PureComponent<Props, State> {
class UnThemedExploreGraphPanel extends PureComponent<Props, State> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor consideration about this panel. In grafana-ui we keep components that are feature agnostic. I think this one, and any other that is Explore specific, should not belong to grafana-ui. Any thoughts on this @ryantxu ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree this does not look like it belongs here. Think Logs visualization should not use ExploreGraphPanel but the Graph component directly.

@@ -1,9 +1,10 @@
import { DataQueryResponse, DataQueryError } from '@grafana/ui';
import { LogRowModel } from '@grafana/data';
import { useState, useEffect } from 'react';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this components. It's very Explore specific, not sure it will ever be used in other context - it belongs to explore feature IMO

Copy link
Contributor Author

@hugohaggmark hugohaggmark Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, but how do we reference this from LogRows which belongs to GrafanaUI? Any suggestions @dprokop @torkelo as a Prop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's tricky indeed. Let's leave it in grafana-ui for now and we will try to figure out a pattern for this behaviour later.

packages/grafana-ui/src/components/Panel/Panel.tsx Outdated Show resolved Hide resolved
@dprokop
Copy link
Member

dprokop commented Aug 21, 2019

I added the label add to changelog grafana/ui as well, is that ok?

We are not keeping grafana-ui changelog yet, but this may be a good start 👍

@hugohaggmark
Copy link
Contributor Author

Found a bug with Show context not showing up in UI, trying to fix it now.

@hugohaggmark
Copy link
Contributor Author

Seems like the rows in Loki context is already broken in master to make that easier to cherrypick I created a separate issue #18656 and I'll create a separate PR for that fix later.

Copy link
Contributor

@davkal davkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • tried in both Explore and as a panel
  • Not a blocker: result processor row limit should respect datasource setting
  • code looks great

Really happy with the progress


if (this.replacePreviousResults) {
return sortedNewResults;
const slice = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This limit should be datasource-dependent.

@hugohaggmark hugohaggmark merged commit e5e7bd3 into master Aug 26, 2019
Observability (deprecated, use Observability Squad) automation moved this from Under review to Done Aug 26, 2019
@hugohaggmark hugohaggmark deleted the hugoh/logs-panel branch August 26, 2019 06:11
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 26, 2019
* grafana/master:
  @grafana/data: improve the CircularVector api (grafana#18716)
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
  Heatmap: Add Cividis and Turbo color schemes (grafana#18710)
  Units: add counts/sec (cps) and counts/min (cpm) in Throughput (grafana#18702)
  Dev Docker: Use golang:1.12.9-alpine to prevent glibc mismatch. (grafana#18701)
  Docs: Fix broken link for the Grafana on RHEL or Ubuntu tutorial (grafana#18697)
  Fixes several usability issues with QueryField component  (grafana#18681)
  convert teams section of user profile to react (grafana#18633)
  Singlestat/Gauge/BarGauge: Improvements to decimals logic and added test dashboard (grafana#18676)
  Emails: Change text (grafana#18683)
  Streaming: improve JSDocs for DataSourceAPI streaming support (grafana#18672)
  TimeSrv: Enable value time windowing in TimeSrv (grafana#18636)
  Explore: Fixes so Show context shows results again (grafana#18675)
  Graph: Updated y-axis ticks test dashboard (grafana#18677)
  Add typings to package.json in packages (grafana#18640)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Aug 28, 2019
* grafana/master:
  Explore: Use PanelQueryState to handle querying (grafana#18694)
  Chore: Improve err message for notifications (grafana#18757)
  @grafana/toolkit: add package versions to the ci report (grafana#18751)
  @grafana/data: Matchers and Transforms (grafana#16756)
  Docs: Document LDAP config reload in admin http api (grafana#18739)
  center NoDataSourceCallToActionCard in Explore (grafana#18752)
  DataLinks: enable data links in Gauge, BarGauge and SingleStat2 panel (grafana#18605)
  DashboardDatasource: reuse query results within a dashboard (grafana#16660)
  Plugins: show a clear error on the plugin page when it failed to load (grafana#18733)
  Chore: Use ruleId instead of alertId as log keyword (grafana#18738)
  @grafana/data: improve the CircularVector api (grafana#18716)
  QueryEditor: check if optional func toggleEditorMode is provided (grafana#18705)
  Emails: remove the yarn.lock (grafana#18724)
  OAuth: Support JMES path lookup when retrieving user email (grafana#14683)
  Emails: resurrect template notification (grafana#18686)
  Email: add reply-to and direct attachment (grafana#18715)
  Dashboard: Adds Logs Panel (alpha) as visualization option for Dashboards (grafana#18641)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Logs Panel
5 participants