-
Notifications
You must be signed in to change notification settings - Fork 12
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
Document the Athena data source for Grafana #50
Conversation
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
|
||
After creating a variable, you can use it in your Athena queries by using [Variable syntax](https://grafana.com/docs/grafana/latest/variables/syntax/). For more information about variables, refer to [Templates and variables](https://grafana.com/docs/grafana/latest/variables/). | ||
|
||
### Annotations |
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.
Should we remove this section for now? We can add this once #17 is implemented.
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.
Rather than deleting sections, write something to acknowledge there there is an absence of annotations. That will help others remember to change that section as new development occurs.
Please consider aligning with this as much as possible: |
Thanks @osg-grafana. I've been pointed to this other template in the past: https://docs.google.com/document/d/1Z4BAybW9K66UHZbI4euwiwyKe3-A4kS8pLAnG-85ACs/edit#. Should we deprecate that one then? (Also note that this is not an enterprise plugin, but I guess there is no adaptation for the doc for public plugins). |
Continue on the path that you are on. We on the Docs Squad need to reconcile these two pieces before advising folks on what to do. Thank you for pointing this document out. I had forgotten about it amidst all the Google Docs. |
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.
Looks good to me! I have some suggestions but they're pretty minor/non-blocking/potentially personal preference, feel free to only add if you agree with them.
README.md
Outdated
|
||
#### Timeseries / Graph visualizations | ||
|
||
For timeseries / graph visualizations, there are a few requirements: |
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.
Maybe a line saying that while all timeseries data can be shown in a table format but not vice versa
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 think that'd be redundant? (we already said that anything can be shown as a table and we are talking here about requirements for timeseries).
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.
Added doc review.
|
||
## Getting started | ||
## Authentication |
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 have created a separate section as discussed.
This should be the URL once my PR is merged: https://grafana.com/docs/grafana/latest/datasources/aws-cloudwatch/aws-authentication/
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.
This has been fixed somewhere in code.
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.
Added few more doc suggestions.
|
||
The backend driver is based on the implementation of [uber/athenadriver](https://github.com/uber/athenadriver), which provides a fully-featured driver for AWS Athena. | ||
### Annotations | ||
|
||
[Annotations](https://grafana.com/docs/grafana/latest/dashboards/annotations/) allow you to overlay rich event information on top of graphs. You can add annotations by clicking on panels or by adding annotation queries via the Dashboard menu / Annotations view. |
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.
Remove/hide this section and table to avoid reader confusion. We can show the docs when the feature is available.
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.
annotations are already available (it just got merged before I finished this PR).
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.
Unblocking
|
||
### Build the frontend | ||
### AWS credentials |
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.
We should remove all authentication related information and simply redirect users to the link I mentioned above.
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.
the link above (https://grafana.com/docs/grafana/latest/datasources/aws-cloudwatch/aws-authentication/) is still giving me a 404, has the related PR been merged already?
aws_secret_access_key = dasdasdsadasdasdasdsa | ||
region = us-west-2 | ||
``` | ||
|
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.
Remove content till 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.
Few more, then we should be good to go.
@achatterjee-grafana I'm going to merge this PR since I need to keep adding things in other. I will update the content once grafana/grafana#39012 is merged. |
Fixes #21
Basic docs with the current status of the plugin. We can add more details / examples as we add new features.