MSSQL Modernization #1450

schmikei · 2025-06-11T18:03:52Z

Utilizing common-lib, logs-lib and grafonnet for most of the mixin generation
Following inspiration for more recently updated mixins like Vsphere

This involves separating out the MSSQL Overview dashboard to separate logs and metrics. This would require some new screenshots.

v-zhuravlev · 2025-06-18T18:09:11Z

mssql-mixin/rows.libsonnet

+    database:
+      [
+        g.panel.row.new('Database'),
+        this.grafana.panels.databaseWriteStallDurationPanel
+        + g.panel.timeSeries.gridPos.withW(12),
+        this.grafana.panels.databaseReadStallDurationPanel
+        + g.panel.timeSeries.gridPos.withW(12),
+        this.grafana.panels.transactionLogExpansionsPanel
+        + g.panel.timeSeries.gridPos.withW(24),
+      ],
+  },


best if row panels are added not as array but inside
g.panel.row.withPanels() function. This way it is possible to toggle rows collapsed/uncollapsed.

example:
https://github.com/grafana/jsonnet-libs/blob/master/kafka-observ-lib/rows.libsonnet#L25-L33.
and then:
https://github.com/grafana/jsonnet-libs/blob/master/kafka-observ-lib/dashboards.libsonnet#L33-L52

Dasomeone

Couple minor comments but overall this looks great to me!
Admittedly a bit rusty on this what with PTO and all so leaving final approval up to @v-zhuravlev

Dasomeone · 2025-06-30T10:33:51Z

mssql-mixin/dashboards_out/mssql_pages

Missing .json filetype on this and the mssql_overview files

Dasomeone · 2025-06-30T10:35:15Z

mssql-mixin/dashboards_out/mssql_pages

+            "keepTime": true,
+            "title": "MSSQL Overview",
+            "type": "link",
+            "url": "/d/mssql_mssql_overview"


I know this is generated, but it feels like a weird change to have mssql twice in the URL?

schmikei · 2025-07-01T18:33:08Z

@v-zhuravlev just curious, do you think we should just default to using signals for this PR as well? I think the API is nice to use and gives us more flexibility probably moving forward.

v-zhuravlev · 2025-07-02T06:37:00Z

@v-zhuravlev just curious, do you think we should just default to using signals for this PR as well? I think the API is nice to use and gives us more flexibility probably moving forward.

yes, it would be great using Signals where possible.

I also see asTarget() as first transitional step for easier migration,

using asTimeSeries() and asPanelMixin(), asTable(), asTableRow() (which propogate units, value mappings from signals) are even better for panel constructions from signals, I encourage you take a look in README or other observ-libs, like kafka, jvm etc.

But let's finish off Clickhouse first.

…odernization

v-zhuravlev

Great work, some comments inline.

v-zhuravlev · 2025-07-15T09:38:14Z

mssql-mixin/config.libsonnet

+  local this = self,
+  enableMultiCluster: false,
+  filteringSelector: 'job=~"integrations/mssql"',
+  groupLabels: ['job', 'cluster'],  // Remove 'db' from here


looks like db is removed.

v-zhuravlev · 2025-07-15T09:39:27Z

mssql-mixin/config.libsonnet

+
+  // logs lib related
+  enableLokiLogs: true,
+  logLabels: ['job', 'cluster', 'instance'],


Suggested change

logLabels: ['job', 'cluster', 'instance'],

logLabels: self.groupLabels + self.instanceLabels,

would it be a better default?

v-zhuravlev · 2025-07-15T09:40:12Z

mssql-mixin/jsonnetfile.json

+      "source": {
+        "git": {
+          "remote": "https://github.com/grafana/grafonnet.git",
+          "subdir": "gen/grafonnet-v10.0.0"


could it be bumped to 11.0.0 or 11.4.0?

v-zhuravlev · 2025-07-15T09:41:03Z

mssql-mixin/main.libsonnet

+        local dbVariable = {
+          label: 'Database',
+          name: 'db',
+          query: 'label_values(mssql_io_stall_seconds_total{%s}, db)' % this.config.filteringSelector,
+          datasource: {
+            type: 'prometheus',
+            uid: '${prometheus_datasource}',
+          },
+          refresh: 2,
+          sort: 1,
+          type: 'query',
+          includeAll: true,
+          allValue: '.*',
+          multi: true,
+        };


why do you need special value here?

I implemented this because only some targets need the db field as part of the filter in the original design.

Old Source:

jsonnet-libs/mssql-mixin/dashboards/mssql-overview.libsonnet

Line 508 in 1cd23af

'100 * mssql_server_total_memory_bytes{' + matcher + '} / clamp_min(mssql_available_commit_memory_bytes{' + matcher + '},1)',

vs

jsonnet-libs/mssql-mixin/dashboards/mssql-overview.libsonnet

Line 590 in 1cd23af

'increase(mssql_io_stall_seconds_total{' + matcher + ', db=~"$database", operation="write"}[$__rate_interval:])',

Dropdown:

jsonnet-libs/mssql-mixin/dashboards/mssql-overview.libsonnet

Lines 892 to 901 in 1cd23af

template.new(

'database',

promDatasource,

'label_values(mssql_log_growths_total{%(mssqlSelector)s, instance=~"$instance"}, db)' % $._config,

label='Database',

refresh=1,

includeAll=true,

multi=true,

allValues='.+',

sort=2

As you can see we got around this last time by selectively using the filter on the specific targets we needed.

With signals it seems a tad bit harder to specify these kind of scenarios where we want a filter dropdown but also want all the filters aside from the db selector. So changing the default value to .* makes it so that the filter still works but does not break other panels when filtering.

Let me know if you think there's an easier way of accomplishing this but this was my solution thus far to keep this going forward!

you just need db var to accept .* as all value?

Right but its moreso that if any database filter is applied then it kind of breaks the other panels if I do it as an instanceLabel...

I suppose is this behavior fine from your perspective? I didn't want to remove the funcitionality of this being a single pane where a user could correlate database data with instance data.

Screen.Recording.2025-07-15.at.12.34.14.PM.mov

if that doesn't make sense let me know but I figured the currently written way was more user friendly, but happy to make the change either way!

ok got it. You check this example with Kafka instance and Kafka instance topics, very similar to yours.

First topic is added(hardcoded) as instance label to topics signals.
https://github.com/grafana/jsonnet-libs/blob/master/kafka-observ-lib/signals/topic.libsonnet#L7
note that for your case it could be instanceLabels + ['db'], there instanceLabels are ommitted for completely different reason.

Second, union of variables generated is used give variables for topics dashboard:
https://github.com/grafana/jsonnet-libs/blob/master/kafka-observ-lib/dashboards.libsonnet#L7-L13

I believe I've implemented based off what you're saying. Let me know if you think its adequate!

v-zhuravlev · 2025-07-15T09:43:46Z

mssql-mixin/signals/connections.libsonnet

+      severeErrors: {
+        name: 'Severe errors',
+        nameShort: 'Severe Errors',
+        type: 'raw',
+        description: 'Rate of severe connection errors per second.',
+        unit: '/ sec',
+        sources: {
+          prometheus: {
+            expr: 'increase(mssql_kill_connection_errors_total{%(queriesSelector)s}[$__rate_interval:])',
+            legendCustomTemplate: '{{ instance }}',
+          },
+        },
+      },


wouldn't it be simplier to use type=counter and simple expression?
Alos, AFAIK, increase doesn't work well with rate_interval. Best if combined just $__interval to avoid increments duplication and wrong "Total" aggregations in the legend etc.

v-zhuravlev · 2025-07-15T09:44:02Z

mssql-mixin/signals/connections.libsonnet

+        name: 'Deadlocks',
+        nameShort: 'Deadlocks',
+        type: 'raw',
+        description: 'Rate of deadlocks per second.',
+        unit: '/ sec',
+        sources: {
+          prometheus: {
+            expr: 'rate(mssql_deadlocks_total{%(queriesSelector)s}[$__rate_interval])',
+            legendCustomTemplate: '{{ instance }}',
+          },


same, why not to use type=counter?

v-zhuravlev · 2025-07-15T09:45:31Z

mssql-mixin/signals/database.libsonnet

+        unit: 'seconds',
+        sources: {
+          prometheus: {
+            expr: 'increase(mssql_io_stall_seconds_total{%(queriesSelector)s, db=~"$db", operation="write"}[$__rate_interval:])',


can db just added as the last element of instanceLabels? Can it be considered the smallest instnance you want to observ (single db within MSSQL host?) would it work?

v-zhuravlev · 2025-07-15T09:45:57Z

mssql-mixin/signals/database.libsonnet

+      },
+      databaseReadStallDuration: {
+        name: 'Database read stall duration',
+        nameShort: 'Read Stall',


Suggested change

nameShort: 'Read Stall',

nameShort: 'Read stall',

Please use Sentence case.

v-zhuravlev · 2025-07-15T09:47:13Z

mssql-mixin/signals/database.libsonnet

+        type: 'raw',
+        description: 'Time spent waiting for database read operations.',
+        unit: 'seconds',
+        sources: {
+          prometheus: {
+            expr: 'increase(mssql_io_stall_seconds_total{%(queriesSelector)s, db=~"$db", operation="read"}[$__rate_interval:])',


Suggested change

type: 'raw',

description: 'Time spent waiting for database read operations.',

unit: 'seconds',

sources: {

prometheus: {

expr: 'increase(mssql_io_stall_seconds_total{%(queriesSelector)s, db=~"$db", operation="read"}[$__rate_interval:])',

type: 'counter',

description: 'Time spent waiting for database read operations.',

unit: 'seconds',

sources: {

prometheus: {

expr: 'mssql_io_stall_seconds_total{%(queriesSelector)s, db=~"$db", operation="read"})',

again, I would defined counters as type=counters and simple expression. See signals README, you can swap rate to increase() as the default function that is applied to the counter if needed.

v-zhuravlev · 2025-07-15T09:47:37Z

mssql-mixin/signals/memory.libsonnet

+      },
+      bufferCacheHitPercentage: {
+        name: 'Buffer cache hit percentage',
+        nameShort: 'Buffer Cache Hit',


Here and similar, please check it is Sentence case.

v-zhuravlev · 2025-07-15T16:33:25Z

mssql-mixin/dashboards_out/mssql_overview.json

-                  "expr": "increase(mssql_kill_connection_errors_total{job=~\"integrations/mssql\",job=~\"$job\",cluster=~\"$cluster\",instance=~\"$instance\"}[$__rate_interval:])",
+                  "expr": "rate(mssql_kill_connection_errors_total{job=~\"integrations/mssql\",job=~\"$job\",cluster=~\"$cluster\",instance=~\"$instance\"}[$__rate_interval])",


some expr got changed function from increase to rate at the end. Just to check that this is what you want. If not, you can change rateFunction to be increase for some signals. See Signals README.

… the database filter

…per viewing

schmikei added 3 commits June 11, 2025 13:59

mssql modernization

613208b

persist makefile

0d6c26c

add prometheus_rules_out

e45ffdf

schmikei marked this pull request as ready for review June 11, 2025 18:34

schmikei requested a review from a team as a code owner June 11, 2025 18:34

v-zhuravlev added the monitoring-mixins label Jun 18, 2025

v-zhuravlev reviewed Jun 18, 2025

View reviewed changes

apply suggestion from review

734601f

Dasomeone reviewed Jun 30, 2025

View reviewed changes

schmikei added 4 commits July 10, 2025 17:00

Merge branch 'master' of github.com:grafana/jsonnet-libs into mssql-m…

4255f66

…odernization

use signals API

508029f

make fmt

4148e1f

fix lint

5b8618b

schmikei requested review from v-zhuravlev and Dasomeone July 14, 2025 20:26

v-zhuravlev reviewed Jul 15, 2025

View reviewed changes

pr feedback; minus the db as instance label

1b35688

v-zhuravlev reviewed Jul 15, 2025

View reviewed changes

schmikei added 3 commits July 15, 2025 13:40

use guidance to adjust query generation so only database signals have…

2cfd3d2

… the database filter

update plugin version to match

11d761c

keep things that were increase as increase but up the minstep for pro…

1f1c2e1

…per viewing

	logLabels: ['job', 'cluster', 'instance'],
	logLabels: self.groupLabels + self.instanceLabels,

	template.new(
	'database',
	promDatasource,
	'label_values(mssql_log_growths_total{%(mssqlSelector)s, instance=~"$instance"}, db)' % $._config,
	label='Database',
	refresh=1,
	includeAll=true,
	multi=true,
	allValues='.+',
	sort=2

		"expr": "increase(mssql_kill_connection_errors_total{job=~\"integrations/mssql\",job=~\"$job\",cluster=~\"$cluster\",instance=~\"$instance\"}[$__rate_interval:])",
		"expr": "rate(mssql_kill_connection_errors_total{job=~\"integrations/mssql\",job=~\"$job\",cluster=~\"$cluster\",instance=~\"$instance\"}[$__rate_interval])",

MSSQL Modernization #1450

Are you sure you want to change the base?

MSSQL Modernization #1450

Uh oh!

Conversation

schmikei commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dasomeone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmikei commented Jul 1, 2025

Uh oh!

v-zhuravlev commented Jul 2, 2025

Uh oh!

v-zhuravlev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-zhuravlev Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmikei commented Jun 11, 2025 •

edited

Loading

v-zhuravlev Jul 15, 2025 •

edited

Loading