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

Add json container logger #1744

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Conversation

wasup-yash
Copy link
Contributor

@wasup-yash wasup-yash commented Oct 6, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR is to implement additional log driver support for the JSON based log driver which enables the JSON log format.

Which issue(s) this PR fixes:

fixes #1126

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Added json container logger support.

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.

Thank you!

Two major points for now:

  1. Please rename JsonInterface to Json, the Interface on ContainerRuntimeInterface is intentional.
  2. You can fix the build by boxing the futures of distinct types. In general, you can use the FutureExt::boxed function to convert the different future types into a single common type using trait objects. This means you can apply the following patch to fix the build:
diff --git a/conmon-rs/server/src/container_log.rs b/conmon-rs/server/src/container_log.rs
index 00363ec8..77bd43af 100644
--- a/conmon-rs/server/src/container_log.rs
+++ b/conmon-rs/server/src/container_log.rs
@@ -2,7 +2,7 @@ use crate::{container_io::Pipe, cri_logger::CriLogger, json_logger::JsonLogger};
 use anyhow::Result;
 use capnp::struct_list::Reader;
 use conmon_common::conmon_capnp::conmon::log_driver::{Owned, Type};
-use futures::future::join_all;
+use futures::{FutureExt, future::join_all};
 use std::sync::Arc;
 use tokio::{io::AsyncBufRead, sync::RwLock};
 
@@ -61,8 +61,8 @@ impl ContainerLog {
             self.drivers
                 .iter_mut()
                 .map(|x| match x {
-                    LogDriver::ContainerRuntimeInterface(ref mut cri_logger) => cri_logger.init(),
-                    LogDriver::JsonInterface(ref mut json_logger) => json_logger.init(),
+                    LogDriver::ContainerRuntimeInterface(ref mut cri_logger) => cri_logger.init().boxed(),
+                    LogDriver::JsonInterface(ref mut json_logger) => json_logger.init().boxed(),
                 })
                 .collect::<Vec<_>>(),
         )
@@ -77,8 +77,8 @@ impl ContainerLog {
             self.drivers
                 .iter_mut()
                 .map(|x| match x {
-                    LogDriver::ContainerRuntimeInterface(ref mut cri_logger) => cri_logger.reopen(),
-                    LogDriver::JsonInterface(ref mut json_logger) => json_logger.reopen(),
+                    LogDriver::ContainerRuntimeInterface(ref mut cri_logger) => cri_logger.reopen().boxed(),
+                    LogDriver::JsonInterface(ref mut json_logger) => json_logger.reopen().boxed(),
                 })
                 .collect::<Vec<_>>(),
         )

conmon-rs/common/proto/conmon.capnp Outdated Show resolved Hide resolved
conmon-rs/server/src/container_log.rs Outdated Show resolved Hide resolved
@saschagrunert
Copy link
Member

May I ask you to rebase on top of the latest main branch to resolve the conflict in conmon-rs/server/Cargo.toml?

@wasup-yash
Copy link
Contributor Author

May I ask you to rebase on top of the latest main branch to resolve the conflict in conmon-rs/server/Cargo.toml?

Hey can you check now, as only one check is failing which is ci/lint-rustfmt(pull_request)

@saschagrunert
Copy link
Member

@wasup-yash can you apply cargo fmt to the PR to format the sources?

@wasup-yash
Copy link
Contributor Author

@wasup-yash can you apply cargo fmt to the PR to format the sources?

done

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2023

Codecov Report

Merging #1744 (4759d60) into main (cec69fa) will increase coverage by 0.83%.
The diff coverage is 46.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1744      +/-   ##
==========================================
+ Coverage   36.33%   37.17%   +0.83%     
==========================================
  Files          13       14       +1     
  Lines        1153     1267     +114     
  Branches      373      417      +44     
==========================================
+ Hits          419      471      +52     
- Misses        495      531      +36     
- Partials      239      265      +26     

@wasup-yash
Copy link
Contributor Author

@wasup-yash can you apply cargo fmt to the PR to format the sources?

How do i resolve these failing tests now? which is ci / test-integration (pull_request) and ci / go-lint (pull_request)

@saschagrunert
Copy link
Member

Can you try golangci-lint run --fix and commit the changes?

@saschagrunert
Copy link
Member

Now we're at:

Warning: var-naming: const LogDriverTypeJsonLogger should be LogDriverTypeJSONLogger (revive)

@wasup-yash
Copy link
Contributor Author

Now we're at:

Warning: var-naming: const LogDriverTypeJsonLogger should be LogDriverTypeJSONLogger (revive)

yup updated that only maybe it'll fix

@wasup-yash
Copy link
Contributor Author

Now we're at:

Warning: var-naming: const LogDriverTypeJsonLogger should be `LogDriverTypeJSONLogger` (revive)

yup updated that only maybe it'll fix

seems like need to change it golangci-lint run --fix ran this but i think need to change it manually to LogDriverTypeJSONLogger

conmon-rs/common/proto/conmon.capnp Outdated Show resolved Hide resolved
conmon-rs/common/proto/conmon.capnp Show resolved Hide resolved
conmon-rs/server/Cargo.toml Outdated Show resolved Hide resolved
conmon-rs/server/src/container_log.rs Show resolved Hide resolved
conmon-rs/server/src/container_log.rs Outdated Show resolved Hide resolved
conmon-rs/server/src/container_log.rs Outdated Show resolved Hide resolved
@saschagrunert saschagrunert changed the title updated for json logger Added json container logger Dec 6, 2023
@saschagrunert saschagrunert changed the title Added json container logger Add json container logger Dec 6, 2023
Signed-off-by: wasup-yash <trooper0018080@gmail.com>
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert
Copy link
Member

saschagrunert commented Dec 6, 2023

@haircommander @rphillips PTAL

@haircommander
Copy link
Collaborator

/approve
/lgtm

thanks @wasup-yash and thanks @saschagrunert for helping out here

Copy link
Contributor

openshift-ci bot commented Dec 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, wasup-yash

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:

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

@openshift-ci openshift-ci bot added the approved label Dec 7, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit f83b1d6 into containers:main Dec 7, 2023
33 checks passed
@saschagrunert saschagrunert deleted the logger branch December 8, 2023 08:10
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.

Add support for additional log drivers
5 participants