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

Feature: offline_log_viewer updates #6233

Closed

Conversation

andijcr
Copy link
Contributor

@andijcr andijcr commented Aug 25, 2022

Cover letter

These changes add a first support in tools/offlile_log_viewer for serde-encoded controller messages.

running

python viewer.py --type controller --path [redpanda data path]

will decode logs produced by v22.2 and earlier. serde commands start with a field 'envelope' that contains
[version, compat_version, encoded_size].

Partially fixes #5293,

implements:

  • serde decode of controller commands:
    • decode_topic_command
    • decode_user_command
    • decode_acl_command
    • decode_config_command
    • decode_feature_command
    • decode_node_management
  • gold master and coverage testing
  • ductape integration
    • test that new version of redpanda can be completely decoded with this tool
    • add means to run this tool as a part of a ductape test

open questions:

  • should there be a check (with a message) that all the bytes are processed?
  • should the structure of the json reflect more closely the cpp class hierarchies (some hierarchies are flattened)?
  • should the names of the fields match the one used in the cpp source (some names are abbreviated or de-abbreviated)?

Backport Required

  • not a bug fix
  • issue does not exist in previous branches
  • papercut/not impactful enough to backport
  • v22.2.x
  • v22.1.x
  • v21.11.x

UX changes

messages are decoded in a stream of json files. the schema for adl encoded files is unchanged, but the schema for serde - encoded message is different in some places than the corresponding adl-one, reflecting more the hierarchical data organization.

Release notes

  • none

@CLAassistant
Copy link

CLAassistant commented Aug 25, 2022

CLA assistant check
All committers have signed the CLA.

@andijcr andijcr added the kind/enhance New feature or request label Aug 25, 2022
…er.read_envelope to accept a type_read Callable[rdr: Reader, version: Int]: Dict and a max_version parameter.

 If a type_read is used, it is invoked with reader and envelope if envelope.version <= max_version parameter.
 the result is joined with a dict {'envelope': envelope} and returned, otherwise an error dict is returned.

 If no type_read is used, envelope is returned immediately
@andijcr andijcr force-pushed the feat/offline_log_viewer_updates branch from 263c60d to 885238d Compare August 31, 2022 17:04
@andijcr andijcr marked this pull request as ready for review August 31, 2022 17:15
@andijcr andijcr requested a review from a team as a code owner August 31, 2022 17:15
@andijcr andijcr requested review from nk-87 and removed request for a team August 31, 2022 17:15
the new code tries to mimic the structure of the adl decoding functions.
the new code is aware of the version number embedded in the binary stream and will skip the field if the version is not supported, while reporting the error.
…update_command) and decode_node_management_command
…rocessed

in case of unread byte, an 'unread': {'k': ..., 'v': ...} field is added and an error is logged

this check is not exact but is a good proxy to know if the decoding code is correct
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

The code in this PR generally looks good, but it's tough to know if we are causing any issues. Can we wire the log viewer into a ducktape test so that as we make changes we are getting automated tests?

@jcsp
Copy link
Contributor

jcsp commented Sep 20, 2022

@andijcr can you merge #5315 into this branch as a starting point for tests? I just rebased that PR but I expect it to fail without the changes in this PR to add serde support.

@jcsp
Copy link
Contributor

jcsp commented Sep 20, 2022

(didn't review the code but I notice the commit messages aren't in the usual format: see CONTRIBUTING.md in the source and/or take a peek at the commit history)

@jcsp
Copy link
Contributor

jcsp commented Nov 24, 2022

@andijcr this has been stalled for a while -- it's not our top priority, but it would be good to know if there were issues blocking this

@jcsp
Copy link
Contributor

jcsp commented Dec 2, 2022

I've merged this into #5315

@jcsp jcsp closed this Dec 2, 2022
@andijcr andijcr deleted the feat/offline_log_viewer_updates branch December 19, 2022 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tools: Update offline_log_viewer for serde-encoded controller logs & test it
5 participants