jhannes / logevents

An easy-to-extend implementation of SLF4J with batteries included and sensible defaults
Other
42 stars 8 forks source link

Support for Humio with Elasticsearch Bulk API #56

Closed norrs closed 2 years ago

norrs commented 2 years ago

Added simple authentication support via providing an Authorization header on the request for NetUtils.

This is not to be confused with Basic Auth, if you need basic auth you need to remember to provide its configuration value as "Basic base64encodedsecrethere".

Wiremock added as dependency to provide some stubbed expectations on the humio responses.

Also borrowed the Proxy code from AbstractHttpPostJsonLogEventObserver, this is however untested.

https://library.humio.com/stable/docs/ingesting-data/log-shippers/other-log-shippers/#elasticsearch-bulk-api

norrs commented 2 years ago

WIP because I need to test it properly

norrs commented 2 years ago

@jhannes Cleaned up and tested using a snapshot of this build in our internal product. Let me know if you need or want any changes.

norrs commented 2 years ago

@jhannes PTAL - I've left conversations open as I assume you prefer to be the one closing em?

I will squash the fixes at the end after we're done with all the feedback, so please do not pay attention to the microcommits and messages they contain.

norrs commented 2 years ago

@jhannes wrote:

Also note that the ElasticsearchLogEventObserver that you are extending is a bit dusty and doesn't fully reflect the current standard of other observers. I'd like your input on how it could be improved in general

Regarding this, I noticed this as well.

My first try yesterday evening was to see if I could adjust the AbstractHttpPostJsonLogEventObserver to be something generic that could work as AbstractHttpPostContentTypeLogEventObserver , but I got stuck with thinking about what is the right way modify the following signatures:

    protected String postJson(Map<String, Object> jsonMessage) throws IOException {
        return NetUtils.postJson(url, JsonUtil.toIndentedJson(jsonMessage), proxy);
    }

    /**
     * Override this method to customize how the {@link LogEventBatch} will be
     * formatted as JSON.
     */
    protected abstract Map<String, Object> formatBatch(LogEventBatch batch);

If it simply were jackson, this could simply have been a JsonNode which can represent both a Json array and a Json object . I'm not sure how you represent that with your internal json library.

Im not sure either if this is the correct way of turning the abstract class into something that can specify a content type, then having some kind of formatter factory which can do something smart depending on the content type.. but maybe? Hm.

norrs commented 2 years ago

I dont mind if you run a linter/autocodeformatter as Im pretty sure you have different code style that my default intellij. (in regards of import etc)

jhannes commented 2 years ago

I can fix the formatting but I didn't find any deviations. I think I'd like to inline EMPTY_LIST_SINCE_HUMIO_API_HAS_NO_SANE_BULK_API_RESPONSE_CONTAINING_DOCUMENT_IDS_FROM_INSERT as the name doesn't add much to the equation, Also elasitcsearch is misspelled. I can fix this after merging. I will accept the pull request now, but you wanted to squash the commits?

I'm expecting to create a new release sometime this week, so your pull request will be included.

Regarding org.logevents.observers.AbstractHttpPostJsonLogEventObserver#postJson, the argument can be changed to Object og we can make two copies of the method.

(As a design philosophy: I have elected to avoid dependencies, including Jackson as these have a risk of interfering with other versions of the same dependency in client code. If there is major JSON work needed to implement an Observer, it's likely that this observer would be better extracted to an extra package. The exceptions for the dependencies are ApplicationInsighs which is an optional dependency in a "opt" package and jansi, which is optional and falls back to a sane default when it's not available)

norrs commented 2 years ago

@jhannes Great, and thanks for accepting the contribution - let us avoid maintaining a fork or make our own module release internally.

And as a owner of a library, it is your rules, and your project which I totally respect. If you have decided to avoid dependencies, great. 👍 Never let anyone come and say "FIX THIS, DO THIS, WHATEVER". If someone wants to deviate from your thoughts, they are free to fork or make their own way.

And thanks for fixing the additional spelling errors and javadoc nits.

In regards of squashing, yes - perfect 👍 Those micro commits had no value in themselves without being helpful during review so you did not have to review the same lines of code when changes are submitted if you rebase and force push the feature branch. Hence I prefer squashing it at the end or reorganize the commits after feedback has been solved.

Have a nice weekend!

jhannes commented 2 years ago

Thank you for a good contribution and patience in getting it to fit. I would be grateful for further contributions 🤗