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

Microsoft SQL Server data source #11298

Merged
merged 49 commits into from Mar 19, 2018

Conversation

Projects
None yet
7 participants
@marefr
Member

marefr commented Mar 19, 2018

Continuing and finalizing work from PR #10093

  • Merged the latest changes from master and fixed conflicts due to Grafana v5.0 and recent changes in postgres/mysql datasources.
  • Added support for $__timeGroup macro function with fill support
  • Added docker blocks for mssql and mssql_tests (grafana/fake-data-gen have been updated).
  • Added more integration tests
  • Documentation will be handled in separate PR targeting docs 5.1 branch
  • Note: No logo included right now. As soon as we have a valid logo we'll fix that
  • Encrypt mssql connection password in database
  • Removes dynamic construction of metric column and other columns. This seems like a niche feature which can be solved by defining multiple queries. In the future we'll probably want to add support for defining series name by alias field similar to how other datasources have solved that, e.g. prometheus.

Fake data:

  • You'll find a dashboard.json in docker/blocks/mssql that you can import into Grafana which are using fake data from grafana/fake-data-gen when you run mssql docker-compose.

Integration tests:

  • You'll find a dashboard.json in docker/blocks/mssql_tests/ that you can import into Grafana after you've created a mssql test datasource and run the integration tests.

Other:

  • Fixed pre-commit so that gofmt only are run for files in pkg directory. Earlier it ran for vendor directory as well which could make you think that dep ensure did some strange things.

linuxchips and others added some commits Dec 2, 2017

mssql: additional integration tests
Metric query of table having multiple value columns
Annotation query

@marefr marefr requested review from xlson, daniellee and bergquist Mar 19, 2018

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 19, 2018

Codecov Report

Merging #11298 into master will decrease coverage by 0.31%.
The diff coverage is 21.26%.

@@            Coverage Diff             @@
##           master   #11298      +/-   ##
==========================================
- Coverage   51.73%   51.41%   -0.32%     
==========================================
  Files         352      354       +2     
  Lines       25344    25607     +263     
  Branches     1492     1492              
==========================================
+ Hits        13111    13166      +55     
- Misses      11507    11703     +196     
- Partials      726      738      +12

codecov-io commented Mar 19, 2018

Codecov Report

Merging #11298 into master will decrease coverage by 0.31%.
The diff coverage is 21.26%.

@@            Coverage Diff             @@
##           master   #11298      +/-   ##
==========================================
- Coverage   51.73%   51.41%   -0.32%     
==========================================
  Files         352      354       +2     
  Lines       25344    25607     +263     
  Branches     1492     1492              
==========================================
+ Hits        13111    13166      +55     
- Misses      11507    11703     +196     
- Partials      726      738      +12
@xlson

xlson approved these changes Mar 19, 2018

Looks good.

Show outdated Hide outdated docker/blocks/mssql/build/Dockerfile
Show outdated Hide outdated pkg/tsdb/mssql/mssql.go

marefr added some commits Mar 19, 2018

mssql: remove dynamic construction of metric column and other columns
This seems like a niche feature which can be solved by defining multiple queries. In the future
we'll probably add support for defining series name by alias field similar to how other datasources
have solved that, e.g. prometheus.
@svenklemm

This comment has been minimized.

Show comment
Hide comment
@svenklemm

svenklemm Mar 19, 2018

Collaborator

I think it would be nice if all sql datasources had somewhat consistent macros. Postgres doesnt require the AS time alias if you use $__timeGroup since the macro already generates it. I think it makes sense if SQL Server datasource would do the same.
See #10074

Collaborator

svenklemm commented Mar 19, 2018

I think it would be nice if all sql datasources had somewhat consistent macros. Postgres doesnt require the AS time alias if you use $__timeGroup since the macro already generates it. I think it makes sense if SQL Server datasource would do the same.
See #10074

@marefr

This comment has been minimized.

Show comment
Hide comment
@marefr

marefr Mar 19, 2018

Member

@svenklemm thanks for the comment. I totally agree, but we need to add some additional logic to render queries differently based on whether macro is used in select, group by or order by. Reason is that you cannot use timestamp as time in a group/order by statement in mssql. This however works in postgres. Second, mssql only supports ORDER BY 1, 2 etc, but not GROUP BY 1, 2. Postgres of course supports both.

First step is to get mssql merged to master. Second, we want to refactor mysql a bit so that the implementation is similar to postgres and mssql. Third, we really want to try and refactor postgres, mysql and mssql since there are a lot of code duplication right now and they're all very similar.

Member

marefr commented Mar 19, 2018

@svenklemm thanks for the comment. I totally agree, but we need to add some additional logic to render queries differently based on whether macro is used in select, group by or order by. Reason is that you cannot use timestamp as time in a group/order by statement in mssql. This however works in postgres. Second, mssql only supports ORDER BY 1, 2 etc, but not GROUP BY 1, 2. Postgres of course supports both.

First step is to get mssql merged to master. Second, we want to refactor mysql a bit so that the implementation is similar to postgres and mssql. Third, we really want to try and refactor postgres, mysql and mssql since there are a lot of code duplication right now and they're all very similar.

@svenklemm

This comment has been minimized.

Show comment
Hide comment
@svenklemm

svenklemm Mar 19, 2018

Collaborator

You cant do that in postgres either but the $__timeGroup macro is intended to be used as a column since the output is the timestamp value for grafana. In the group by you use backreferences to refer to the output of $__timeGroup like GROUP BY 1. Changing it later would break queries people have written with the old version of the macro. Not sure if SQL Server supports backreferences though.

Collaborator

svenklemm commented Mar 19, 2018

You cant do that in postgres either but the $__timeGroup macro is intended to be used as a column since the output is the timestamp value for grafana. In the group by you use backreferences to refer to the output of $__timeGroup like GROUP BY 1. Changing it later would break queries people have written with the old version of the macro. Not sure if SQL Server supports backreferences though.

@marefr

This comment has been minimized.

Show comment
Hide comment
@marefr

marefr Mar 19, 2018

Member

Yes, see my updated comment. You cannot use backreference in mssql in group by expression which is a pity.

Member

marefr commented Mar 19, 2018

Yes, see my updated comment. You cannot use backreference in mssql in group by expression which is a pity.

@svenklemm

This comment has been minimized.

Show comment
Hide comment
@svenklemm

svenklemm Mar 19, 2018

Collaborator

Oh well that sucks. I guess not producing the alias is indeed the best way to do it for now until some other solution is found.

Collaborator

svenklemm commented Mar 19, 2018

Oh well that sucks. I guess not producing the alias is indeed the best way to do it for now until some other solution is found.

@marefr

This comment has been minimized.

Show comment
Hide comment
@marefr

marefr Mar 19, 2018

Member

Yes unfortunately.

Maybe possible to do the changes later without breaking change if we can be backward compatible and handle the cases when there is a AS column after a macro function - shouldn't be that hard.

Thanks a lot for your review and wise thoughts @svenklemm

Member

marefr commented Mar 19, 2018

Yes unfortunately.

Maybe possible to do the changes later without breaking change if we can be backward compatible and handle the cases when there is a AS column after a macro function - shouldn't be that hard.

Thanks a lot for your review and wise thoughts @svenklemm

@daniellee daniellee merged commit 4033283 into master Mar 19, 2018

4 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-backend Your tests passed on CircleCI!
Details
ci/circleci: test-frontend Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
@@ -119,7 +119,7 @@
"git add"
],
"*.go": [
"gofmt -w -s",
"gofmt -w -s pkg",

This comment has been minimized.

@bergquist

bergquist Mar 20, 2018

Contributor

I dont think this works as expected. husky adds the filepath to the end of this cmd. So this becomes
gofmt -w -s pkg /what/ever/files/you/changes.go
I think a better fix would be to only check go files in pkg/**.go

@bergquist

bergquist Mar 20, 2018

Contributor

I dont think this works as expected. husky adds the filepath to the end of this cmd. So this becomes
gofmt -w -s pkg /what/ever/files/you/changes.go
I think a better fix would be to only check go files in pkg/**.go

This comment has been minimized.

@marefr

marefr Mar 20, 2018

Member

Good catch. Will fix in master straight away!

@marefr

marefr Mar 20, 2018

Member

Good catch. Will fix in master straight away!

This comment has been minimized.

@marefr

marefr Mar 20, 2018

Member

Good catch. Will fix in master straight away!

@marefr

marefr Mar 20, 2018

Member

Good catch. Will fix in master straight away!

This comment has been minimized.

@bergquist

bergquist Mar 20, 2018

Contributor

No rush :)

@bergquist

bergquist Mar 20, 2018

Contributor

No rush :)

@bergquist bergquist deleted the mssql_datasource branch Mar 20, 2018

@daniellee

This comment has been minimized.

Show comment
Hide comment
@daniellee

daniellee Mar 20, 2018

Member

Thanks @linuxchips for all your hard work - this is a fantastic feature for Grafana.

Member

daniellee commented Mar 20, 2018

Thanks @linuxchips for all your hard work - this is a fantastic feature for Grafana.

@marefr marefr referenced this pull request Mar 20, 2018

Closed

Add MSSQL logo #11305

marefr added a commit that referenced this pull request Mar 20, 2018

ryantxu added a commit to NatelEnergy/grafana that referenced this pull request Mar 20, 2018

Merge remote-tracking branch 'grafana/master'
* grafana/master: (137 commits)
  snapshot: fix legend rendering bug
  session: update defaults for ConnMaxLifetime
  snapshots: removes errors for empty values in ViewStore
  Expose option to disable snippets
  changed var to const, changed to string interpolation
  Remove unused kibana images
  changed var to const
  changelog: adds note about closing #11114 & #11086
  converted file to ts
  changelog: notes about #10093 and #11298
  dashboard: fix phantomjs panel rendering in collapsed row
  converted file to ts
  fix: only run gofmt on pkg directory omitting vendor directory
  fix: dep ensure. now without gofmt on ventor directory
  mssql: add integration test to verify stored procedure usage
  mssql: encrypt password in database
  mssql: remove dynamic construction of metric column and other columns
  docker: pin microsoft/mssql-server-linux to 2017-CU4 tag
  docs: improve guide for high availability
  fix: dep ensure. now without gofmt on ventor directory
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment