Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reformat and redesign the recording output. #186

Merged

Conversation

terjekv
Copy link
Collaborator

@terjekv terjekv commented Dec 6, 2023

Changes

  • Formats command events into properly related JSON.
  • Properly record status responses (ok, warning, errors).
  • Record API requests as a chain related to each command.
  • Record the raw command issued, the actual command executed, and the command filter applied (if any).
  • Moves comment handling to OutputManager.
  • Consolidates stop_recording and save_recording.
  • Renamed some variables and methods for consistency.
  • Timestamps and runtime (in milliseconds) for commands are recorded, but may be disabled with --record-without-timestamps.

Issues

  • The stack details for errors and warnings is bugged. See log. _prefix_from_stack() needs a defined behaviour. #185
  • We buffer the entire recording blob in memory until we stop recording (or we exit). This is to ensure valid JSON for the output, as writing (valid) JSON streams to a file is non-trivial. A solution is to write after each command, reload the file and append the data, then write again. This may or may not be a good idea, and it would look something like:
            existing_data = []

            # Read the existing data from the file
            try:
                with open(self._filename, "r") as rec_file:
                    existing_data = json.load(rec_file)
            except (FileNotFoundError, json.JSONDecodeError):
                pass

            existing_data.append(recording_entry)

            # Write the updated data back to the file
            with open(self._filename, "w") as rec_file:
                json.dump(existing_data, rec_file, indent=2)

Modified tests

As this completely redefines the recording output, tests results are recreated.

Fixes #183

@terjekv terjekv self-assigned this Dec 6, 2023
@terjekv terjekv force-pushed the terjekv/Improve-the-format-for-recordings branch from f76bf46 to 009e22e Compare December 6, 2023 21:18
@terjekv
Copy link
Collaborator Author

terjekv commented Dec 6, 2023

Before:

[
  {
    "command": "zone list -forward\n"
  },
  {
    "method": "GET",
    "url": "/api/v1/zones/forward/",
    "data": {},
    "status": 200,
    "response": {
      "count": 1,
      "next": null,
      "previous": null,
      "results": [
        {
          "nameservers": [
            {
              "name": "ns1.example.org",
              "ttl": null
            }
          ],
          "updated": false,
          "primary_ns": "ns1.example.org",
          "email": "[email protected]",
          "refresh": 10800,
          "retry": 3600,
          "expire": 1814400,
          "soa_ttl": 43200,
          "default_ttl": 43200,
          "name": "example.org"
        }
      ]
    }
  },
  {
    "output": "Zones:\n   example.org"
  },
  {
    "command": "zone set_ns example.org ns2.example.org"
  },
  {
    "method": "GET",
    "url": "/api/v1/hosts/ns2.example.org",
    "data": {},
    "status": 404,
    "response": {
      "detail": "Not found."
    }
  },
  {
    "method": "GET",
    "url": "/api/v1/hosts/?cnames__name=ns2.example.org",
    "data": {},
    "status": 200,
    "response": {
      "count": 0,
      "next": null,
      "previous": null,
      "results": []
    }
  },
  {
    "output": "WARNING: : host not found: 'ns2.example.org'"
  },
  {
    "output": "WARNING: : ns2.example.org is not in mreg, must force"
  }
]

After:

[
  {
    "command": "zone list -forward",
    "command_filter": null,
    "command_issued": "zone list -forward",
    "ok": [],
    "warning": [],
    "error": [],
    "output": [
      "Zones:",
      "   example.org"
    ],
    "api_requests": [
      {
        "method": "GET",
        "url": "/api/v1/zones/forward/",
        "data": {},
        "status": 200,
        "response": {
          "count": 1,
          "next": null,
          "previous": null,
          "results": [
            {
              "nameservers": [
                {
                  "name": "ns1.example.org",
                  "ttl": null
                }
              ],
              "updated": false,
              "primary_ns": "ns1.example.org",
              "email": "[email protected]",
              "refresh": 10800,
              "retry": 3600,
              "expire": 1814400,
              "soa_ttl": 43200,
              "default_ttl": 43200,
              "name": "example.org"
            }
          ]
        }
      }
    ]
  },
  {
    "command": "zone set_ns example.org ns2.example.org",
    "command_filter": null,
    "command_issued": "zone set_ns example.org ns2.example.org  #  requires force because ns2 is unknown",
    "ok": [],
    "warning": [
      "WARNING: : host not found: 'ns2.example.org'",
      "WARNING: : ns2.example.org is not in mreg, must force"
    ],
    "error": [],
    "output": [],
    "api_requests": [
      {
        "method": "GET",
        "url": "/api/v1/hosts/ns2.example.org",
        "data": {},
        "status": 404,
        "response": {
          "detail": "Not found."
        }
      },
      {
        "method": "GET",
        "url": "/api/v1/hosts/?cnames__name=ns2.example.org",
        "data": {},
        "status": 200,
        "response": {
          "count": 0,
          "next": null,
          "previous": null,
          "results": []
        }
      }
    ]
  }
]

Note that this moves warnings (and errors) from the "output" block to separate blocks. This can be merged for output if desired.

@terjekv
Copy link
Collaborator Author

terjekv commented Dec 6, 2023

Note that console output is unchanged.

mreg_cli/outputmanager.py Outdated Show resolved Hide resolved
mreg_cli/outputmanager.py Outdated Show resolved Hide resolved
@terjekv terjekv force-pushed the terjekv/Improve-the-format-for-recordings branch 2 times, most recently from 5aaea87 to 5ad7947 Compare December 12, 2023 08:54
@terjekv terjekv marked this pull request as ready for review December 12, 2023 09:35
  - Formats command events into properly related JSON.
  - Properly record status responses (ok, warning, errors).
  - Record API requests as a chain related to each command.
  - Record the raw command issued, the actual command executed, and the command filter applied (if any).
  - Moves comment handling to OutputManager.
  - Consolidates stop_recording and save_recording.
  - Renamed some variables and methods for consistency.

  - We want timestamps in the logs, but they will differ for CI runs and thus fail. There needs to be a configuration option to disable logging timestamps...
  - The stack details for errors and warnings is bugged. See unioslo#185
  - We buffer the entire recording blob in memory until we stop recording (or we exit). This is to ensure valid JSON for the output, as writing (valid) JSON streams to a file is non-trivial. A solution is to write after each command, reload the file and append the data, then write again. This may or may not be a good idea, and it would look something like:

```python
            existing_data = []

            # Read the existing data from the file
            try:
                with open(self._filename, "r") as rec_file:
                    existing_data = json.load(rec_file)
            except (FileNotFoundError, json.JSONDecodeError):
                pass

            existing_data.append(recording_entry)

            # Write the updated data back to the file
            with open(self._filename, "w") as rec_file:
                json.dump(existing_data, rec_file, indent=2)
```

As this completely redefines the recording output, tests fail miserably.
  - Recording-related methods now all start with recording_ as a prefix.
  - Added an option to enable or disable timing data from the recorded data.
  - In general, callers adding details to the recorded data shouldn't test if recording is active. OutputManager handles that detail, the rest of the code simply states that it wishes to make something available for recording.
  - RecordingEntry is given proper type.
  - TimeInfo in RecordingEntry is given proper type.
  - Correct typing declarations during init.
@terjekv terjekv force-pushed the terjekv/Improve-the-format-for-recordings branch from 5ad7947 to 5cc6c1e Compare December 13, 2023 09:35
@terjekv terjekv requested a review from pederhan December 13, 2023 10:12
mreg_cli/outputmanager.py Outdated Show resolved Hide resolved
mreg_cli/types.py Outdated Show resolved Hide resolved
@terjekv terjekv merged commit 263a668 into unioslo:master Dec 18, 2023
11 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.

Improve the format for recordings.
2 participants