Skip to content

feat(odin): Register fetch logs action and validate input dates#541

Merged
vikramlc-cognite merged 4 commits into
masterfrom
EDGE-605-register-fetch-logs-action
Jun 26, 2026
Merged

feat(odin): Register fetch logs action and validate input dates#541
vikramlc-cognite merged 4 commits into
masterfrom
EDGE-605-register-fetch-logs-action

Conversation

@vikramlc-cognite

@vikramlc-cognite vikramlc-cognite commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the first step of the bulk log upload feature for Odin: registers a built-in fetch_logs action on every extractor instance and validates the incoming date-range parameters before any file I/O is attempted.


Type of change

  • New feature (non-breaking change that adds functionality)

What changed

  • ActionError exception class (actions.py): New exception for structured action failures. Carries error_type (machine-readable category) and optional details, both surfaced as result_metadata in the Odin action update. _handle_custom_action now catches ActionError before the generic Exception clause so structured metadata is preserved on the update.
  • _log_upload_action.py (new private module): Contains fetch_logs_action - validates start_date / end_date from ctx.call_metadata (ISO 8601, inclusive, max 7 days). Raises ActionError with error_type of missing_parameter, invalid_parameter, or invalid_date_range on any validation failure.
  • Extractor._register_builtin_actions() (base.py): New private method called unconditionally in __init__ (between __init_tasks__ and __init_actions__). Registers fetch_logs on every extractor without requiring subclass opt-in. add_action now raises ValueError on duplicate name to prevent subclasses from shadowing built-ins.

Why it changed

Operators need to trigger log retrieval from the Odin UI without any per-extractor code changes. Registering as a built-in ensures the action is available on all extractors uniformly.


Related docs / discussion:

Design Doc - Bulk Log Upload Action


What to focus on during review

  • _register_builtin_actions is called in __init__ before __init_actions__. Confirm this ordering is safe for all existing extractor subclasses (subclasses that call add_action in __init_actions__ will see fetch_logs already present).
  • The except ActionError branch in _handle_custom_action must sit before except Exception - any reordering silently swallows structured metadata.
  • add_action duplicate-name guard: raises ValueError at registration time, not at dispatch time. Intentional - fail fast rather than silently overwrite.

Test evidence

  • tests/test_unstable/test_log_upload_action.py (new, 11 test cases): registration, all missing-param combinations, invalid ISO formats, invalid date ranges, boundary at exactly 7 days, ActionError.details wiring.
  • tests/test_unstable/test_action_dispatch.py: added test_action_error_sets_result_metadata_and_keeps_failed_status covering the except ActionError branch in dispatch.
  • tests/test_unstable/test_action_registration.py: updated 6 existing tests to be semantic (name/type lookups) rather than count-based, since the built-in adds one action to every extractor.
  • All 43 tests pass; ruff clean.

Risks and unknowns

  • File I/O and CDF Files upload are not yet implemented - fetch_logs_action returns successfully after validation. A valid date range call currently does nothing beyond parameter checking. Subsequent PRs complete the implementation (Tasks 2–5).
  • Log file path detection (Task 2) depends on the extractor's configured file handler - the approach for extractors with no file handler is not yet finalised.

Rollout and rollback

  • No config changes or API changes required. Rolling back means reverting this commit; no data is written to CDF at this stage. Once Tasks 2–5 land, rollback would require ensuring no in-flight uploads are orphaned.

Checklist

  • Self-reviewed the diff
  • Tests added or updated
  • Docs updated - N/A (design doc is pre-existing)
  • No secrets, credentials, or PII committed
  • Breaking changes called out - none

@vikramlc-cognite vikramlc-cognite self-assigned this Jun 19, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a built-in fetch_logs action to stream rotated log files to CDF Files, validating date ranges up to a maximum of 7 days. It also adds a structured ActionError exception class to report structured metadata on action failures, integrates this built-in action registration and error handling into the base extractor, and includes comprehensive tests. Feedback on the changes suggests catching both ValueError and TypeError in _parse_date to ensure robust input validation when non-string types are passed in the metadata.

Comment thread cognite/extractorutils/unstable/core/_log_upload_action.py
@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.86%. Comparing base (8fd8266) to head (cb33f35).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   82.73%   82.86%   +0.12%     
==========================================
  Files          44       45       +1     
  Lines        4430     4475      +45     
==========================================
+ Hits         3665     3708      +43     
- Misses        765      767       +2     
Files with missing lines Coverage Δ
...extractorutils/unstable/core/_log_upload_action.py 100.00% <100.00%> (ø)
cognite/extractorutils/unstable/core/actions.py 100.00% <100.00%> (ø)
cognite/extractorutils/unstable/core/base.py 84.59% <100.00%> (+0.29%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vikramlc-cognite vikramlc-cognite marked this pull request as ready for review June 22, 2026 04:06
@vikramlc-cognite vikramlc-cognite requested a review from a team as a code owner June 22, 2026 04:06
Comment thread cognite/extractorutils/unstable/core/_log_upload_action.py
Comment on lines +351 to 361
def _register_builtin_actions(self) -> None:
"""Register framework-level actions available on every extractor."""
self.add_action(
CustomAction(
name="fetch_logs",
target=fetch_logs_action,
description=_FETCH_LOGS_DESCRIPTION,
)
)

def __init_actions__(self) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit conflicting, its a built-in action being regarded as a custom action, which is a bit weird

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can also have CogniteActions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch_logs uses the identical dispatch mechanism as user-registered actions (callable + ActionContext). The only difference is registration time (automatic in init vs explicit in init_actions). Introducing a parallel type (CogniteActions) hierarchy for the same runtime behaviour adds complexity without changing anything observable.

assert len(req.available_actions) == 3
names = [a.name for a in req.available_actions]
assert names == ["Start sync", "Stop sync", "flush"]
# Built-in actions precede scheduled-task start/stop, which precede user custom actions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't validate the order of custom actions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code puts scheduled task start/stop first, then _custom_actions in registration order (built-ins first, user actions after). But agree an assert can be added here. Added.

@vikramlc-cognite vikramlc-cognite requested a review from nithinb June 24, 2026 09:57
@vikramlc-cognite vikramlc-cognite added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Jun 25, 2026

@erlendvollset erlendvollset left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦄

@erlendvollset erlendvollset added risk-review-ongoing Risk review is in progress waiting-for-team Waiting for the submitter or reviewer of the PR to take an action and removed waiting-for-risk-review Waiting for a member of the risk review team to take an action labels Jun 26, 2026
@erlendvollset erlendvollset self-assigned this Jun 26, 2026
@vikramlc-cognite vikramlc-cognite merged commit c61ce2a into master Jun 26, 2026
7 checks passed
@vikramlc-cognite vikramlc-cognite deleted the EDGE-605-register-fetch-logs-action branch June 26, 2026 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-review-ongoing Risk review is in progress waiting-for-team Waiting for the submitter or reviewer of the PR to take an action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants