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

Support named handlers #156 #210

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

treivize
Copy link
Contributor

@treivize treivize commented Oct 4, 2023

I guess named handlers are managed as expected with a defaulting for any config property to the root splunk configuration.

I was a bit sad about having had to duplicate the properties from the root level and the named handler ones, but I did not find how to customize the default value to detect in the recorder when to fetch the root value or the handler value when a default value is defined upfront. Any idea?

Fixes #156

@treivize treivize force-pushed the feat/support-named-handlers branch 7 times, most recently from cbcc57a to bfb2bd1 Compare October 6, 2023 08:06
@treivize
Copy link
Contributor Author

treivize commented Oct 6, 2023

Finally, as discussed with @rquinio1A, the config at named handler level will not default on the root level, but will be handled independently only reusing the root level defaulting values.

There is no more config property duplications now.

@treivize treivize force-pushed the feat/support-named-handlers branch from bfb2bd1 to 327ce54 Compare October 6, 2023 08:17
@rquinio1A
Copy link
Member

rquinio1A commented Oct 6, 2023

Finally, as discussed with @rquinio1A, the config at named handler level will not default on the root level, but will be handled independently only reusing the root level defaulting values.

There is no more config property duplications now.

It could indeed have been interesting that named handlers inherit their configuration from the root/default handler.
However we've seen that:

  • For fields, you can't differentiate a default value from @ConfigItem annotation vs explict configuration of the user using the default value, so you don't know whether root handler config should be used...
  • Quarkus built-in handlers (file, console, ...) don't seem to do this, so it could be confusing for users

Copy link
Member

@rquinio1A rquinio1A left a comment

Choose a reason for hiding this comment

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

There's the generated config included in the doc, but it could you add a small doc section under features (https://github.com/quarkiverse/quarkus-logging-splunk/blob/main/docs/modules/ROOT/pages/index.adoc#features) with an example config and typical code usage like in the unit test ?
Few points to highlights:

  • Every handler is isolated and uses a separate Splunk client and connection pool, which means it has a cost.
  • The configuration from the root handler are not inherited by named handlers.
  • `quarkus.log.category."named-handler".use-parent-handlers=false is required if you do not want the root handler to also receive log events already sent to named handlers.

@treivize treivize force-pushed the feat/support-named-handlers branch from 327ce54 to f232a2b Compare October 6, 2023 12:11
@treivize treivize requested a review from rquinio1A October 6, 2023 12:13
@rquinio1A rquinio1A merged commit c34f62c into quarkiverse:main Oct 6, 2023
1 check passed
@rquinio1A
Copy link
Member

@treivize Thanks for the contribution!

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.

Support named handlers
2 participants