LogstashBasicMarker implementation is inefficient and out-off-sync with implementation provided by Slf4J

[brenuart]

LogstashBasicMarker is a now obsolete copy of org.slf4j.helpers.BasicMarker at version 1.7.12
The original implementation has been updated and improved since then. The version included in logback-logstash-encoder should be updated accordingly.

A few points:

• replace the use of Vector by a more efficient List implementation
• use a StringBuilder instead of a StringBuffer

Note: Slf4J BasicMarker has a package protected constructor and cannot be easily re-used in other project - reason why the project maintains its own copy.

[brenuart]

Instead of copying an alternative is to declare LogstashBasicMarker in the org.slf4j.helpers package with a public constructor (inside this project) and extend from it.

@philsttr What do you think of this? Would it cause issues in OSGI environments?

[philsttr]

Instead of copying an alternative is to declare LogstashBasicMarker in the org.slf4j.helpers package with a public constructor (inside this project) and extend from it.

It was originally in org.slf4j.helpers, but was moved into net.logstash.logback.marker to fix #104 . I think it should stay in net.logstash.logback.marker

[brenuart]

I see. So better to leave it like this and keep an "internal" copy.

This class is public and behaves the same as the SLF4J version. Users may be tempted to use it as base class for their own specialisations... Is it something we want to support? Or should we protect it?
Personally, I would instead merge everything inside LogstashMarker and have only "one" base class for all of our markers.

Finally, we "copied" the class... but don't you think we should copy the relevant tests as well ?

[philsttr]

Is it something we want to support?

No

Or should we protect it?

Yes, probably worth reducing the visibility.

but don't you think we should copy the relevant tests as well ?

definitely

[brenuart]

Yes, probably worth reducing the visibility.

@philsttr Or even better - collapse the two implementations into one and keep only the LogstashMarker...

[philsttr]

I kept them separate to easily see the differences between our copied version and the original... primarily so it would make it easier to sync.

However, if the logstash-logback-encoder implementation is going to start diverging more, then I don't see a reason to keep them separate.