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

Support discrete events (e.g. annotations) #5

Closed
wants to merge 22 commits into from

Conversation

retzkek
Copy link
Contributor

@retzkek retzkek commented Mar 29, 2017

The primary changes in this PR are to add support for discrete events coming from a datasource like Elasticsearch (I think that's the only one that supports the "docs" type), where each event has a title, start time, and end time. This example shows the same data source being used for annotations and events:

image

Other changes included (can be cherry-picked or split into separate PRs if desired):

  • Tooltips are shown for the correct row
  • Show legend entries for all rows/metrics
  • Configure all the things!

Some of this probably could use some tweaks, but I figured it was good enough to start a dialog.

retzkek added 12 commits March 28, 2017 12:08
When metric type is "docs" (e.g. "raw documents" from Elasticsearch),
treat each document as an event, with a given start and end time.

Currently hardcoded to use the following fields in the document:
begin - start time
end - end time
desc - name
This will override the "from" range when querying the
datasource, causing it to return data from the epoch.
This is needed in case an event started before the graph start.
* Show legend entries for each metric (instead of just the first).
* Optionally show metric names next to legend.
* Use event name for value.
* Fix dates in tooltips for events.
@retzkek
Copy link
Contributor Author

retzkek commented Mar 29, 2017

Options editor:

image

Colors editor:

image

Tooltips & Legend (timeseries data):

image

@ryantxu
Copy link
Contributor

ryantxu commented Mar 29, 2017

nice -- i'll try to look more closely tomorrow. If you make a PR for just:

  • Tooltips are shown for the correct row
  • Show legend entries for all rows/metrics
  • Configure all the things!

I will apply that quickly.

Whats the story with "Query all data"? Why not just keep the time range out of the metric query?

@retzkek
Copy link
Contributor Author

retzkek commented Mar 29, 2017

Opened #6 with the tooltip fix and the config options.

The issue with using the panel time range is I want to also show "ongoing" events, that have one or both of start and end dates outside the displayed range. The data source is (most likely) indexed on the start time, so anything earlier wouldn't get included. It's a bit of a hack, but I haven't come up with a better option.

@ryantxu
Copy link
Contributor

ryantxu commented Mar 30, 2017

Ahh, i see similar to this issue:
influxdata/influxdb#5943

I have not used elastic, but in lucene/solr, you can index your dates using a DateRangeField
https://lucidworks.com/2016/02/13/solrs-daterangefield-perform/

and this solves the issue

@retzkek
Copy link
Contributor Author

retzkek commented Mar 30, 2017

Elasticsearch doesn't have a date range field, but even if it did the problem is Grafana includes a range filter against the configured time field in every query.
The other option I see (without modifying the Grafana datasource code) is to directly go to Elasticsearch with a custom query through datasource._post() rather than datasource.query(), but then it's Elasticsearch-specific, while other datasources may support the docs type at some point. It would allow for a "correct" query that only fetches relevant documents though, e.g. $beginField:[* TO $timeTo] AND $endField:[$timeFrom TO *]

@ryantxu
Copy link
Contributor

ryantxu commented Mar 30, 2017

@retzkek, can you update the PR? I merged and cleaned up #6

thanks!

@ryantxu
Copy link
Contributor

ryantxu commented Apr 12, 2017

@retzkek, i'm back from vacation and can take a look... can you get things to merge again?

@retzkek
Copy link
Contributor Author

retzkek commented Apr 12, 2017

Sorry @ryantxu, I've had to focus on some other things. I'm still sort of mulling over whether to stick with the "all data" option or go with an elasticearch-specific query. I'm worried what happens if someone has a lot of events... we only have a couple hundred, growing by a couple per week, so it works for our needs, but not a great general solution.

@ryantxu
Copy link
Contributor

ryantxu commented Apr 12, 2017

no worries -- for sure i would look into an indexing strategy that lets you do this on the server. The 'all data' option is a bit funky, and I don't get why you can't just do that by removing the time query from the Metric query

@retzkek
Copy link
Contributor Author

retzkek commented Apr 12, 2017

I don't get why you can't just do that by removing the time query from the Metric query

The Elasticsearch datasource always includes a range filter in the query:
https://github.com/grafana/grafana/blob/master/public/app/plugins/datasource/elasticsearch/query_builder.js#L174

It also has a fixed size limit of 500:
https://github.com/grafana/grafana/blob/master/public/app/plugins/datasource/elasticsearch/query_builder.js#L112

So current solution won't work anyways.

@ryantxu
Copy link
Contributor

ryantxu commented Apr 13, 2017

Check grafana/grafana#8098

looks like the concept of a 'region' is coming to annotations (time & timeTo)

@retzkek
Copy link
Contributor Author

retzkek commented Apr 13, 2017

Oh neat, thanks for the heads up. That might actually solve the problem I originally set out to solve with this... Although having a separate panel still has its uses.

@cherweg
Copy link

cherweg commented Jun 22, 2017

Thats a really cool feature! Even if grafana has it´s own annotations.
Please merge :-)

regards
Christian

@ryantxu
Copy link
Contributor

ryantxu commented Jun 27, 2017

@retzkek are you going to merge this with master? or should we close it and open a new issue?

@retzkek
Copy link
Contributor Author

retzkek commented Jun 28, 2017

@ryantxu If you're interested in merging I'll take a pass at resolving conflicts. I don't want to spend too much time though due to the issues mentioned above (although we are using it in production...).

@ryantxu
Copy link
Contributor

ryantxu commented Jun 29, 2017

Lets see what happens with the range (event) annotations, and then go from there. I'll close this now and we can pick up a new issue if that makes sense

@ryantxu ryantxu closed this Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants