Refactor some JsonProviders for use with both logging and access events #663
Conversation
|
@philsttr Please have a first look at this PR when you have time... The documentation lists the available providers in two sections: one with the providers applicable to ILoggingEvent and a second for IAccessEvent. I think it can be clearer if we move the providers applicable to both in a third section that would appear before the two other. Also the documentation says provider named |
Sounds good. Make sure to link to the common section from both of the logging/access event specific sections
Yes, you are correct |
philsttr
added
the
warn/api-change
|
Added the not-backward-compatible label since this change is not binary compatible (it looks like it should be source-compatible though) |
|
Conventions in the code base seems to prefix abstract classes with "Abstract" (I quite like it - this make it obvious it is an intermediate base class that cannot be used directly). To follow that convention, I renamed @philsttr Question: should I repeat the static fields in the now deprecated class (eg AbstractFormattedTimestampJsonProvider.java#L40)? IMHO it is not strictly required...
|
... only |
There was a problem hiding this comment.
Question: should I repeat the static fields in the now deprecated class (eg AbstractFormattedTimestampJsonProvider.java#L40)? IMHO it is not strictly required...
To be binary-compatible, they would need to remain. To be source-compatible, they can be removed. I have tried to keep logstash-logback-encoder as-compatible-as-possible in the past, since that is best for its users. But for 7.0 we've already broken compatibility a lot, so I'm ok with just keeping source-compatibility here. We need to keep an eye on binary and source compatibility in future releases. I don't want logstash-logback-encoder to get into the habit of breaking compatibility in lots of places all the time.
CompositeJsonFormatter is another candidate... Should I rename it as well? And keep a deprecated version of it?
In a non-major release, I would say no (unless we kept full binary compatibility). But you can rename it and keep source-compatibility only since 7.0 is a major release.
README.md
Outdated
| The table below lists the available providers for _LoggingEvents_, and their configuration properties (defaults in parentheses). | ||
| The provider name is the xml element name to use when configuring. |
There was a problem hiding this comment.
Mention and provide a link to the common providers section somewhere in this paragraph.
Perhaps:
| The table below lists the available providers for _LoggingEvents_, and their configuration properties (defaults in parentheses). | |
| The provider name is the xml element name to use when configuring. | |
| The [common providers mentioned above](#providers-common-to-loggingevents-and-accessevents), and the providers listed in the table below, are available for _LoggingEvents_. | |
| The provider name is the xml element name to use when configuring. Each provider's configuration properties are shown, with default configuration values in parenthesis. |
README.md
Outdated
| The table below lists the available providers for _AccessEvents_, and their configuration properties (defaults in parentheses). | ||
| The provider name is the xml element name to use when configuring. |
There was a problem hiding this comment.
Mention and provide a link to the common providers section somewhere in this paragraph.
Perhaps:
| The table below lists the available providers for _AccessEvents_, and their configuration properties (defaults in parentheses). | |
| The provider name is the xml element name to use when configuring. | |
| The [common providers mentioned above](#providers-common-to-loggingevents-and-accessevents), and the providers listed in the table below, are available for _AccessEvents_. | |
| The provider name is the xml element name to use when configuring. Each provider's configuration properties are shown, with default configuration values in parenthesis. |
Refactor UuidProvider and associate utility classes to make them usable for both ILoggingEvent and IAccessEvent: - create a new UuidJsonProvider by making a copy of the existing UuidProvider (restricted to ILoggingEvent) - deprecate the existing UuidProvider in favour of the common UuidJsonProvider - move the LoggingEventJsonProviders#addUuid() method into the common JsonProviders class Closes #662
…vent Refactor SequenceJsonProvider and associate utility classes to make them usable for both ILoggingEvent and IAccessEvent: - create a new SequenceJsonProvider by making a copy of the existing SequenceJsonProvider (restricted to ILoggingEvent) - deprecate the existing SequenceJsonProvider in favour of the new common SequenceJsonProvider - move LoggingEventJsonProviders#addSequence() method into the common JsonProviders class Closes #661
…sEvent Refactor the existing ThreadNameJsonProvider and associate utility classes to make them usable for both ILoggingEvent and IAccessEvent. Closes #660
…ng and Access *JsonProviders
Keep and deprecate existing `FormattedTimestampJsonProvider` class until next major version
These classes are now deprecated. Emit a WARN status if ever they are used in a XML configuration.
…section + sort providers alphabetically
Rename `CompositeJsonFormatter` into `AbstractCompositeJsonFormatter` to comply with the "abstract" naming convention.
brenuart
force-pushed
the
gh662-refactor-providers-for-logging-and-access
branch
from
e5ff137
to
2a25705
Compare
brenuart
deleted the
gh662-refactor-providers-for-logging-and-access
branch
This PR addresses issues #660 , #661 and #662.