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

Allow FieldNames to override the field name of ContextJsonProvider when used in the context of IAccessEvent #659

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

brenuart
Copy link
Collaborator

Closes #658

@brenuart
Copy link
Collaborator Author

@philsttr Looks like the field thread could also be promoted to a common name. The ThreadNameJsonProvider could then be used for both logging and access events.

Similarly, SequenceJsonProvider and UuidProvider are currently limited to ILoggingEvent but may potentially be useful in the context of IAccessEvent as well.

@philsttr
Copy link
Collaborator

philsttr commented Sep 23, 2021

@philsttr Looks like the field thread could also be promoted to a common name. The ThreadNameJsonProvider could then be used for both logging and access events.

The *FieldNames is only needed/used for the providers that are in LogstashAccessEncoder and LogstashEncoder by default. *FieldNames is needed because the default providers of those encoders are not configured individually (like is done for the *CompositeJsonEncoder). Since ThreadNameJsonProvider is not in LogstashAccessEncoder by default, it doesn't need to be in *FieldNames.

For comparison, when the *CompositeJsonEncoders are used directly, *FieldNames is not used. Instead, the field name used by each provider is configured directly on each provider, as opposed to being configured on the encoder level.

Similarly, SequenceJsonProvider and UuidProvider are currently limited to ILoggingEvent but may potentially be useful in the context of IAccessEvent as well.

Yes, they can be refactored to handle IAccessEvent or ILoggingEvent, but they still wouldn't need to to be in *FieldNames, since they would not be in LogstashAccessEncoder or LogstashEncoder by default.

@brenuart
Copy link
Collaborator Author

brenuart commented Sep 23, 2021

Ok - got it for Field Names.

@philsttr Should we open separate issues to refactor ThreadNameJsonProvider, SequenceJsonProvider and UuidProvider to be usable for both ILoggingEvent and IAccessEvents ?
I'm not convinced many people will use them with IAccessEvents (should anybody use access events at all...) but this could make the code base consistent. It wasn't clear to me at first sight why these providers were limited to logging events... In fact, there is no reason except nobody asked for it ;-)

@philsttr
Copy link
Collaborator

Yes, please open an issue for them. I agree we should make them consistent. Thanks :)

@brenuart brenuart merged commit 76465c9 into main Sep 23, 2021
@brenuart brenuart deleted the gh658-field-names branch September 23, 2021 19:59
@philsttr philsttr added this to the 7.0 milestone Nov 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LogstashAccessEncoder#fieldNames does not change the field name used by ContextJsonProvider
2 participants