Skip to content

Skip telemetry writes when no database is configured - #66

Merged
nickhuo merged 2 commits into
mainfrom
jiajunh/can-283-deploy-logs-a-sqlalchemy-parse-error-every-5s-when-no
Sep 9, 2026
Merged

Skip telemetry writes when no database is configured#66
nickhuo merged 2 commits into
mainfrom
jiajunh/can-283-deploy-logs-a-sqlalchemy-parse-error-every-5s-when-no

Conversation

@nickhuo

@nickhuo nickhuo commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

If we assume the database URL is a prerequisite, we can ignore this pr. @Saaketh0 @iidsample

With no database configured, every metrics poll tried to build a SQLAlchemy engine from the string ‎"None". It failed the same way every 5 seconds — ‎Could not parse SQLAlchemy URL from given URL string — burying the errors operators actually need to see.

Skip telemetry when there’s no database and fix URL resolution

With no `database` in the controller config, every metrics poll built an
engine from the string "None", so SQLAlchemy raised "Could not parse
SQLAlchemy URL from given URL string" once per instance every 5 seconds.

Telemetry has nowhere to go without a database, so the writes -- and the
metrics that only exist to feed them -- are now skipped, and the fact is
stated once at startup.
@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f799a7d0-599c-40cd-905a-a326421f5ecc


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nickhuo nickhuo changed the title [CAN-283] Skip telemetry writes when no database is configured Skip telemetry writes when no database is configured Sep 3, 2026

Saaketh0 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

This doesn't look like the OTLP exporter branch? The telemetry writes you are skipping won't be used in the future

Saaketh0 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

I just tagged Felipe somewhere about this but I can't find it lol, but if UI doesn't pull from that DB anymore you can just remove the code instead of adding a skip file from the telemetry_logger.py

Saaketh0 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

The agent metrics doesn't currently have a OTLP reciever though so just remove the runtime_information table logic

@nickhuo

nickhuo commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

This doesn't look like the OTLP exporter branch? The telemetry writes you are skipping won't be used in the future

What is SQLAlchemy used for here?

The purpose of this PR is to keep the log from polling every five seconds and flooding other error msg

@nickhuo
nickhuo marked this pull request as ready for review September 9, 2026 22:29
…nfigured

Resolves conflicts from the Ventis->CanyonOS rename (CAN-255) landing on
main after this branch's CAN-283 fix:

- telemetry_logging.py: keep resolve_database_url()'s "empty means no
  database" behavior, updated to read CANYONOS_DATABASE_URL (renamed from
  VENTIS_DATABASE_URL on main).
- global_controller.py: adopt main's parallelized per-instance polling
  (_poll_one_instance via ThreadPoolExecutor) and its new OTel row export,
  layering in the database-configured guard around the legacy
  send_runtime_information/send_agent_information calls so OTel export
  keeps working even without a database while the legacy SQL writes stay
  skipped. Dropped the now-redundant _write_telemetry helper and the
  removed _agent_host_key in favor of instance_manager._routing_endpoint_for.
- project_id default: took main's `config.get("project_id")` (no default),
  matching main's `_load_config` which now always mints one.
- Updated tests/test_global_controller_telemetry_skip.py for the
  canyonos_core module paths, the CANYONOS_DATABASE_URL env var, and the
  new _poll_one_instance entry point.
@nickhuo
nickhuo merged commit 9c9820f into main Sep 9, 2026
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants