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.

Sign up for GitHub

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

Jump to bottom

Add util class to support to add annotations to Grafana #378

Merged
merged 12 commits into from
Apr 15, 2019
Merged

Add util class to support to add annotations to Grafana #378

merged 12 commits into from
Apr 15, 2019

Conversation

qiffang
Copy link
Contributor

@qiffang qiffang commented Apr 9, 2019 •
edited
Loading

Function

  1. Support to add annotations to Grafana
  2. Add a errorCount metrics to record how many error occurred.
    Prometheus can scrape this metric by xxx:8083/metrics.

Usage

  1. cli, _ := NewGrafanaClient("http://$GrafanaIP:3000", "$GrafanaUserName", "$GrafanaPassword", "$PrometheusScrapePort")
  2. annotation := Annotation{ dashboardId: $dashboardId, panelId: $panelId, tags: []string{"tag"}, timestampInMilliSec: time.Now().Unix() * 1000, text: "test", }
  3. cli.AddAnnotation(annotation)
  4. cli.IncrErrorCount()

Test

  1. Add Unit Test Case - PASS
  2. Check the Grafana Annotation - PASS
    image
  3. Check the metrics by prometheus exporter - PASS
    image

weekface
weekface reviewed Apr 10, 2019
View reviewed changes
pkg/util/annotationUtil.go Outdated Show resolved Hide resolved
pkg/util/annotationUtil.go Outdated

//IncreErrorCountWithAnno increments the errorcount by 1,
//and add the annotation to grafanan.
func (cli *client) IncreErrorCountWithAnno(annotation Annotation) error{
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this method. only addAnnotation is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split it as two functions
1.AddAnnotation
2.IncrErrorCount

pkg/util/annotationUtil.go Outdated Show resolved Hide resolved
aylei
aylei suggested changes Apr 10, 2019
View reviewed changes
Copy link
Contributor

@aylei aylei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Prefer underscore case for file name (e.g. annotation_util.go).
Rest LGTM.

tests/metrics/annotationUtil.go Outdated Show resolved Hide resolved
tests/metrics/annotationUtil.go Outdated Show resolved Hide resolved
tests/metrics/annotationUtil.go Outdated Show resolved Hide resolved
tests/metrics/annotationUtil.go Outdated Show resolved Hide resolved
@weekface weekface added the test/stability stability tests label Apr 11, 2019
@qiffang qiffang requested a review from aylei April 12, 2019 05:55
aylei
aylei reviewed Apr 12, 2019
View reviewed changes
Copy link
Contributor

@aylei aylei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Almost fine, the last concern is some fields are optional and some fields are missing (isRange, timeEnd)

tests/metrics/annotation_util.go Outdated
//Annotation is a specification of the desired behavior of adding annotation
type Annotation struct {
DashboardId int `json: "dashboardId"`
PanelId int `json: "panelId"`
Copy link
Contributor

@aylei aylei Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

DashboardId and PanelId are optional, so we should omit these field is empty (nil for pointer, 0 for numeric type), the notation is json:"dashboardId,omitempty"

Copy link
Contributor

Choose a reason for hiding this comment

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

So do tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dashboardId和pannelId明显是不能忽略的啊，要不然知道加哪张图上
tags accept

Copy link
Contributor

Choose a reason for hiding this comment

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

The scenario is that users may add the global annotation (without DashboardId and PanelId) to grafana and query it in multiple dashboards. To me it is the common case because when I am coding I cannot ensure the grafana dashboard id and panel id is consistent in different clusters and different e2e tests or stability tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we post an annotation without dashboardID and panelID?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, https://grafana.com/docs/http_api/annotations/#create-annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave them optional for a wider range of uses. @qiffang

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the original requirement, we want to add annotation in a graph(panel).
Of course, we can support it.
Accept

aylei
aylei reviewed Apr 12, 2019
View reviewed changes
Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

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

Great! There is a small typo however, rest LGTM.

tests/pkg/metrics/annotation_util.go Outdated Show resolved Hide resolved
@qiffang qiffang requested a review from aylei April 13, 2019 02:38
aylei
aylei approved these changes Apr 13, 2019
View reviewed changes
Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

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

LGTM

weekface
weekface approved these changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@weekface weekface left a comment

Choose a reason for hiding this comment

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

lgtm

@weekface weekface merged commit 37d9c5f into pingcap:master Apr 15, 2019
yahonda pushed a commit that referenced this pull request Dec 27, 2021
@weekface @ran-huang @DanielZhangQD
zh: helm offline document (#378)
5a1b21a 
* helm offline

* fix ci

* tiny fix

* Apply suggestions from code review

Co-authored-by: Ran <huangran@pingcap.com>

* Update zh/tidb-toolkit.md

Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>

Co-authored-by: Ran <huangran@pingcap.com>
Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weekface weekface weekface approved these changes

@aylei aylei aylei approved these changes

Assignees
No one assigned
Labels
test/stability stability tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qiffang @weekface @aylei