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

Make Promtail more friendly for using as a library #2405

Closed
rfratto opened this issue Jul 23, 2020 · 3 comments
Closed

Make Promtail more friendly for using as a library #2405

rfratto opened this issue Jul 23, 2020 · 3 comments

Comments

@rfratto
Copy link
Member

rfratto commented Jul 23, 2020

I'm working on embedding Promtail into grafana/agent and while my prototype works, there's a few things I'd like to change in Promtail to embed it more cleanly:

  1. Allow passing a custom log.Logger to Promtail rather than using util.Logger. I'd like to log messages using my existing logger with a component=promtail key to make it clear log lines are logging related.
  2. Allow providing a custom mux router and cleanup function to pkg/promtail/targets/lokipush. The Agent creates one server object and I'd prefer to not have to have a second one for the Loki Push API.
  3. Add RegisterFlagsWithPrefix functions to all config structs so I can register flags to a flagset but with a namespaced prefix to make it clear they're logging related.

I'd like to make separate PRs to handle all of these, but wanted to open an issue first to collect feedback first. I think passing a custom logger and router is best implemented through the functional options pattern. My usage of pkg/promtail would look something like:

promtail.New(cfg, false, promtail.WithLogger(logger), promtail.WithRouter(router))

While cmd/promtail would continue to use it like it already does:

promtail.New(config, *dryRun)

/cc @slim-bean @owen-d @cyriltovena

@owen-d
Copy link
Member

owen-d commented Jul 23, 2020

All ideas seem good to me, although I admit I'm not super familiar with (2).

@cyriltovena
Copy link
Contributor

Looks good to me, not sure about the function pattern, I prefer (cfg Config, opt ...optionfunc) or just more parameters. I don’t like mutating stuff after New.

rfratto added a commit to rfratto/loki that referenced this issue Jul 24, 2020
All config structs now have a RegisterFlagsWithPrefix function that
allows specifying a prefixed string to append to all flag names. This
may be used by clients to namespace support for Promtail in a larger
application (e.g., grafana/agent).

When implementing this PR, I noticed a few config structs were
incorrectly registering to the global flagset rather than the flagset
passed to RegisterFlags. This PR also includes a fix for that and
ensures every option is registered to the flagset in the parameter.

Related to grafana#2405.
owen-d pushed a commit that referenced this issue Jul 27, 2020
All config structs now have a RegisterFlagsWithPrefix function that
allows specifying a prefixed string to append to all flag names. This
may be used by clients to namespace support for Promtail in a larger
application (e.g., grafana/agent).

When implementing this PR, I noticed a few config structs were
incorrectly registering to the global flagset rather than the flagset
passed to RegisterFlags. This PR also includes a fix for that and
ensures every option is registered to the flagset in the parameter.

Related to #2405.
rfratto added a commit to rfratto/loki that referenced this issue Jul 28, 2020
This commit allows for creating promtail with a custom log.Logger
instance which will be propagated and used consistently throughout the
Promtail package. This allows for clients to provide a Promtail-specific
logger.

Address a portion of grafana#2405.
owen-d pushed a commit that referenced this issue Jul 28, 2020
…ly (#2438)

* pkg/promtail: propagate a logger rather than using util.Logger globally

This commit allows for creating promtail with a custom log.Logger
instance which will be propagated and used consistently throughout the
Promtail package. This allows for clients to provide a Promtail-specific
logger.

Address a portion of #2405.

* remove applyOptions function
@rfratto
Copy link
Member Author

rfratto commented Aug 11, 2020

I'm going to close this as implemented now, with the exception of this:

Allow providing a custom mux router and cleanup function to pkg/promtail/targets/lokipush. The Agent creates one server object and I'd prefer to not have to have a second one for the Loki Push API.

At first I didn't realize the user can have an arbitrary number of the loki push targets, each on a different port. I don't think its worth having the Agent just use one server here, since I'd have to place extra restrictions that doesn't exist in Promtail (i.e., only allow a single push target). Plus, the syslog target definitely needs a separate port, so even if I did have a solution for the push target, there's always going to be at least one other target that depends on multiple ports being opened.

@rfratto rfratto closed this as completed Aug 11, 2020
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

No branches or pull requests

3 participants