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

input: output: Bind log api from fluent-bit #20

Merged
merged 10 commits into from
Jul 5, 2022
Merged

Conversation

cosmo0920
Copy link
Contributor

Related to #19.

I binded the fundamental APIs to use fluent-bit's logging mechanism.
With this patch, we can call flb_log_print and flb_log_check via flb_api interface.

Signed-off-by: Hiroshi Hatake [email protected]

@cosmo0920
Copy link
Contributor Author

@nicolasparada Could you add logging interface in top of this PR?

@cosmo0920 cosmo0920 removed the request for review from niedbalski July 1, 2022 11:31
@cosmo0920
Copy link
Contributor Author

I tried to implement Logger interface for Golang plugin.
Not that this implementation needs to work with the latest fluent/fluent-bit#5056 patch.

@nicolasparada
Copy link
Contributor

nicolasparada commented Jul 4, 2022

Is the log level always required?

We can have something like:

logger.Log(msg, args...)
level.Error(logger).Log(msg, args...)

If we can skip the log level.

@cosmo0920
Copy link
Contributor Author

Oh, good point. In fluent-bit's logging system, log level is always required. But, in Golang world, we can implement a builder for constructing logger.
I'll continue to consider builder pattern in this logger implementation.

@nicolasparada
Copy link
Contributor

That's ok, if it always required in fluent-bit, we can follow that with Go too.
Be aligned in the APIs is important.

cshared.go Outdated
@@ -325,3 +376,7 @@ func makeMetrics(cmp *cmetrics.Context) Metrics {
},
}
}

func GetLogger() Logger {
return logger
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't make this a package level thing.
Maybe Init() method on the plugin could receive it in its args.

Copy link
Contributor

@nicolasparada nicolasparada Jul 4, 2022

Choose a reason for hiding this comment

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

Init() is already receiving too many args.

ctx context.Context
conf plugin.ConfigLoader
metrics plugin.Metrics

And adding plugin.Logger to those... Maybe we can pass a struct with all of those in there.

type Fluentbit struct {
  Conf plugin.ConfigLoader
  Metrics plugin.Metrics
  Logger plugin.Logger
}

Init() can now looks like this:

func (myPlugin) Init(ctx context.Context, fbit plugin.Fluentbit) error {
  fbit.Config.String("some_param")
  fbit.Metrics.NewCounter("my_counter", "desc")
  fbit.Logger.Info("calling init")

  return nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good. I implemented your suggestion. ¡Genial!

Copy link
Contributor

@niedbalski niedbalski left a comment

Choose a reason for hiding this comment

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

@cosmo0920 2 things I would like to add 👍🏻

  1. Please expand the README.md with an example that uses logger.
  2. Please add a test to validate that logger works in the dummy plugin

Thanks!

@cosmo0920 cosmo0920 force-pushed the handle-exposed-log-api branch from f02213b to d6c6972 Compare July 5, 2022 09:18
@cosmo0920
Copy link
Contributor Author

I added the commit to reflect the argument changes on Init

@cosmo0920
Copy link
Contributor Author

cosmo0920 commented Jul 5, 2022

2. Please add a test to validate that logger works in the dummy plugin

I think that we should do in test plugins that is implemented in testdata.

@cosmo0920 cosmo0920 force-pushed the handle-exposed-log-api branch 2 times, most recently from 9d29fd3 to c10404c Compare July 5, 2022 10:20
@cosmo0920
Copy link
Contributor Author

Oh, CI is failed. I'll take a look later.

@cosmo0920 cosmo0920 force-pushed the handle-exposed-log-api branch from c10404c to c01ccbf Compare July 5, 2022 12:26
@cosmo0920
Copy link
Contributor Author

CI got green! 💚

@niedbalski niedbalski merged commit 79d417f into main Jul 5, 2022
@niedbalski niedbalski deleted the handle-exposed-log-api branch July 5, 2022 14:23
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.

3 participants