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

Use capnp map for tracing metadata #1498

Merged

Conversation

mgjm
Copy link
Contributor

@mgjm mgjm commented Jul 11, 2023

What type of PR is this?

/kind other

What this PR does / why we need it:

Currently (if tracing is enabled) the metadata field gets filled with a JSON encoded string map, that contains tracing metadata.

This PR changes this and uses capnp native maps and thereby eliminates the double encoding of JSON data inside of a capnp struct. Note that the proper way to implement maps in capnp is to use a List of string tuples.

Changes:

  • Rename the existing metadata field to metadataOld
  • Add a new metadata field of type List[StringStringMapEntry]
  • Add utility functions to convert between a List of MapEntrys and rust types
  • Remove serde_json from the server code
  • Move capnp util function in go to capnp_util.go (slice and map encoding)
  • Encode the metadata in the client in both formats (for backwards-compatibility)

Compatibility:

  • I don't expect the conmon-rs server installed on a system to be newer than the client implementation.
  • Eher the client and the server are installed with the same version (via os packages or installed from source)
  • Or the client is newer (installed from source) and the server is older (installed via os packages)
  • These cases are supported by the implemented backwards-compatibility in the client
  • In the rare case that the server is on a newer version only tracing does not work (the server ignores the old field, everything else works as expected)

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Are those compatibility requirements acceptable?

Does this PR introduce a user-facing change?

None

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2023

Codecov Report

Merging #1498 (76b82fd) into main (03b7321) will decrease coverage by 0.18%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1498      +/-   ##
==========================================
- Coverage   34.71%   34.53%   -0.18%     
==========================================
  Files          13       13              
  Lines        1135     1135              
  Branches      394      391       -3     
==========================================
- Hits          394      392       -2     
  Misses        483      483              
- Partials      258      260       +2     

@mgjm mgjm force-pushed the refactor-capnp-metadata branch 3 times, most recently from 5af8261 to 3605b28 Compare July 11, 2023 11:01
@mgjm
Copy link
Contributor Author

mgjm commented Jul 11, 2023

Third time's the charm. Fixed now all typos and lints.

@haircommander
Copy link
Collaborator

@mgjm there's a merge conflict can you rebase please?

@haircommander
Copy link
Collaborator

/approve

LGTM

@saschagrunert PTAL

Signed-off-by: Martin Michaelis <code@mgjm.de>
@saschagrunert
Copy link
Member

saschagrunert commented Jul 18, 2023

The linter is not happy:

error: this argument is a mutable reference, but not used mutably
   --> conmon-rs/server/src/child_reaper.rs:364:45
    |
364 |     async fn spawn_cleanup_process(raw_cmd: &mut Vec<String>) {
    |                                             ^^^^^^^^^^^^^^^^ help: consider changing to: `&Vec<String>`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut
    = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings`

That's probably coming from a toolchain update.

@mgjm
Copy link
Contributor Author

mgjm commented Jul 18, 2023

@saschagrunert Yeah, I haven't touched that code. Should I fix it anyway in this PR?

@saschagrunert
Copy link
Member

saschagrunert commented Jul 18, 2023

@saschagrunert Yeah, I haven't touched that code. Should I fix it anyway in this PR?

Yes please, other PRs are affected by this as well.

@mgjm
Copy link
Contributor Author

mgjm commented Jul 18, 2023

Btw, this lint is a false positive (or clippy is smart enough to detect that there is a non mutating alternative):

    async fn spawn_cleanup_process(raw_cmd: &mut Vec<String>) {
        let mut cleanup_cmd = Command::new(raw_cmd.remove(0));

raw_cmd.remove is mutating function. But we can replace this with a slice access.

Signed-off-by: Martin Michaelis <code@mgjm.de>
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, mgjm, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [haircommander,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit b9fafc9 into containers:main Jul 18, 2023
28 checks passed
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.

5 participants