chore: SAP HANA mixin modernization #1550

schmikei · 2025-11-20T19:13:05Z

This PR modernizes the mixin to use g.libsonnet and commonlib where applicable.

It also migrates over to the signals API.

As of submission mostly validated against the git diff for changes.

aalhour

First pass, the only real issue is the alerts, the other stuff are minor.

aalhour · 2025-12-11T15:45:14Z

sap-hana-mixin/dashboards.libsonnet

+          dashboards+:
+            {
+              logs+:
+                root.applyCommon(super.logs.templating.list, uid=uid + '-logs', tags=tags, links=links { couchdbLogs+:: {} }, annotations=annotations, timezone=timezone, refresh=refresh, period=period),


Typo: couchdbLogs --> sapHanaLogs

aalhour · 2025-12-11T15:45:58Z

sap-hana-mixin/links.libsonnet

+
+    sapHanaInstance:
+      link.link.new('SAP HANA instance overview', '/d/' + this.grafana.dashboards['sap-hana-instance-overview.json'].uid)
+      + link.link.options.withKeepTime(true),


Missing withIncludeVars(true) in link.

aalhour · 2025-12-11T15:46:02Z

sap-hana-mixin/links.libsonnet

+  new(this): {
+    sapHanaOverview:
+      link.link.new('SAP HANA system overview', '/d/' + this.grafana.dashboards['sap-hana-system-overview.json'].uid)
+      + link.link.options.withKeepTime(true),


Missing withIncludeVars(true) in link.

aalhour · 2025-12-11T15:47:14Z

sap-hana-mixin/dashboards.libsonnet

+          vars.multiInstance,
+          uid + '-system-overview',
+          tags,
+          links,


Should exclude the current dashboards from the links to avoid self-referencing issues?
{ sapHanaOverview+:: {} }

aalhour · 2025-12-11T15:47:37Z

sap-hana-mixin/dashboards.libsonnet

+          vars.multiInstance,
+          uid + '-instance-overview',
+          tags,
+          links,


Same comment about excluding this dashboard from its own links.

aalhour · 2025-12-11T15:49:18Z

sap-hana-mixin/panels.libsonnet

+        + g.panel.timeSeries.standardOptions.thresholds.withSteps([
+        ])
+        + g.panel.timeSeries.fieldConfig.defaults.custom.withThresholdsStyle({ mode: 'line' }),


Do we need to add thresholds here? In case it's empty?

aalhour · 2025-12-11T15:50:16Z

sap-hana-mixin/signals/system.libsonnet

@@ -0,0 +1,221 @@
+function(this) {
+  local aggregationLabels = std.join(',', this.groupLabels + this.instanceLabels),


Minor nit: sometimes we use ',' and some other times we use ', ' (with a space), can we use one?

aalhour · 2025-12-11T15:50:29Z

sap-hana-mixin/signals/instance.libsonnet

@@ -0,0 +1,163 @@
+function(this) {
+  local aggregationLabels = std.join(', ', this.groupLabels + this.instanceLabels),


Minor nit: sometimes we use ',' and some other times we use ', ' (with a space), can we use one?

aalhour · 2025-12-11T15:51:47Z

sap-hana-mixin/alerts.libsonnet

Can we use without(...) instead of by(...), from the docs:

sum by (job, sid, host) drops labels; prefer sum without (...) to preserve labels for alert routing

Dasomeone

Couple comments here, I think we could make better use of the common-lib, even if just as a replacement for the base panels (stat/table specifically)

Dasomeone · 2025-12-16T10:36:08Z

sap-hana-mixin/config.libsonnet

+  local this = self,
+
+  // Filtering and label configuration
+  filteringSelector: 'job="integrations/sap-hana"',


Please drop integration job label

Dasomeone · 2025-12-17T15:13:50Z

sap-hana-mixin/alerts.libsonnet

+          {
+            alert: 'SapHanaHighSqlExecutionTime',
+            expr: |||
+              avg without (database_name, port, service, sql_type) (hanadb_sql_service_elap_per_exec_avg_ms{%(filteringSelector)s}) / 1000 > %(alertsCriticalHighSqlExecutionTime)s


This is a minor thing, but in conjunction with my other comment on the config:

Suggest we swap the alert threshold variables to milliseconds to match the native unit of the metric itself.

Would apply for both:

alertsCriticalHighSqlExecutionTime

alertsCriticalHighReplicationShippingTime

Dasomeone · 2025-12-17T15:14:29Z

sap-hana-mixin/config.libsonnet

+  alertsCriticalHighSqlExecutionTime: 1,  // second
+  alertsCriticalHighReplicationShippingTime: 1,  // second


Just had a thought: Given the native metrics use milliseconds we could just match that here, would simplify our alert expressions a bit and lower chance of issues during maintenance

Dasomeone · 2025-12-17T15:21:13Z

sap-hana-mixin/rows.libsonnet

No need to specify x,y throughout this file

Dasomeone · 2025-12-17T15:22:06Z

sap-hana-mixin/dashboards.libsonnet

+                this.grafana.rows.instanceResourcesRow,
+                this.grafana.rows.instanceIORow,
+                this.grafana.rows.instancePerformanceRow,
+                this.grafana.rows.instanceAlertsRow,


Generally speaking we'd want the alerts row up top

Dasomeone · 2025-12-17T15:23:43Z

sap-hana-mixin/dashboards.libsonnet

+                this.grafana.rows.systemReplicationRow,
+                this.grafana.rows.systemResourcesRow,
+                this.grafana.rows.systemIORow,
+                this.grafana.rows.systemPerformanceRow,
+                this.grafana.rows.systemAlertsRow,


Probably want to reorganise this with:

resources

alerts

replication

performance

io

Thoughts?

I'm not 100% on the order of rep/per/io specifically, but I do think resources and alerts should be at the top

Dasomeone · 2025-12-17T15:24:13Z

sap-hana-mixin/dashboards.libsonnet

+                this.grafana.rows.instanceResourcesRow,
+                this.grafana.rows.instanceIORow,
+                this.grafana.rows.instancePerformanceRow,


Same thing as the system dashboard, generally speaking I think:

Resources

Alerts

the remainder, in a similar pattern as whatever we agree on for system above

Dasomeone · 2025-12-17T15:26:19Z

sap-hana-mixin/panels.libsonnet

+      systemReplicaStatusPanel:
+        g.panel.stat.new('Replica status')
+        + g.panel.stat.queryOptions.withTargets([
+          signals.system.replicaStatus.asTarget(),
+        ])
+        + signals.system.replicaStatus.asOverride()
+        + g.panel.stat.panelOptions.withDescription('State of the replicas in the SAP HANA system')
+        + g.panel.stat.options.withColorMode('value')
+        + g.panel.stat.options.withGraphMode('none')
+        + g.panel.stat.options.withTextMode('auto')
+        + g.panel.stat.standardOptions.thresholds.withMode('absolute')
+        + g.panel.stat.standardOptions.thresholds.withSteps([
+          { color: 'green', value: null },
+          { color: 'green', value: 0 },
+          { color: 'yellow', value: 1 },
+          { color: 'red', value: 3 },
+        ]),


You're doing a lot of overrides here, does none of the base ones provided in common-lib come close?

I'd try to refrain from invoking g.panel.stat.new directly, and instead go via commonlib.panels.stat.base.new instead, so as to inherit any best practices (which you can then override, if needed)

Would help pull in future changes to our base panels

Dasomeone · 2025-12-17T15:27:06Z

sap-hana-mixin/panels.libsonnet

+        + g.panel.timeSeries.standardOptions.withUnit('ms'),
+
+      systemActiveConnectionsPanel:
+        g.panel.stat.new('Active connections')


Same as above, would refrain from invoking g.panel.stat.new directly unless absolutely necessary.

Applies to all the stat panels in here

Dasomeone · 2025-12-17T15:28:00Z

sap-hana-mixin/panels.libsonnet

+        + g.panel.stat.options.withTextMode('auto'),
+
+      systemRecentAlertsPanel:
+        g.panel.table.new('Recent alerts')


Please use base from https://github.com/grafana/jsonnet-libs/tree/master/common-lib/common/panels/generic/table

Dasomeone

meant to hit "request changes" not just comment 😅

wip SAP HANA mixin modernization

a1becc6

schmikei added the monitoring-mixins label Nov 20, 2025

fix lint with alerts

e7dcb7a

schmikei mentioned this pull request Nov 20, 2025

chore: Modernize sap-hana-mixin #1527

Closed

schmikei marked this pull request as ready for review November 25, 2025 21:32

schmikei requested a review from a team as a code owner November 25, 2025 21:32

aalhour requested changes Dec 11, 2025

View reviewed changes

aalhour self-assigned this Dec 11, 2025

schmikei added 2 commits December 11, 2025 16:14

the easy feedback from most recent review

4a79795

remove filteringSelector based off recent feedback

c7539f1

Dasomeone reviewed Dec 17, 2025

View reviewed changes

Dasomeone requested changes Dec 17, 2025

View reviewed changes

		@@ -0,0 +1,221 @@
		function(this) {
		local aggregationLabels = std.join(',', this.groupLabels + this.instanceLabels),

		@@ -0,0 +1,163 @@
		function(this) {
		local aggregationLabels = std.join(', ', this.groupLabels + this.instanceLabels),

		alertsCriticalHighSqlExecutionTime: 1, // second
		alertsCriticalHighReplicationShippingTime: 1, // second

chore: SAP HANA mixin modernization #1550

Are you sure you want to change the base?

chore: SAP HANA mixin modernization #1550

Uh oh!

Conversation

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

Uh oh!

Uh oh!

aalhour 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!

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!

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!

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!

Dasomeone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

schmikei commented Nov 20, 2025 •

edited

Loading