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

Add json_lines webhook input format #100

Merged
merged 1 commit into from
Sep 7, 2020
Merged

Conversation

JensErat
Copy link
Contributor

@JensErat JensErat commented May 7, 2020

fluent-bit allows two JSON log formats: a stream of JSON objects not separated at all,
and one with a single JSON object per line. None of those was compatible with
grok_exporter so far. This change implements the json_lines format that is compatible
with fluent-bit.

I did not implement for v2 config, is this considered deprecated regarding new features or should the change also be included there? Runs fine with json_lines configuration and fluent-bit in our integration stage with some millions of log records by now.

I'd also be fine adding up some refactoring regarding duplicate code, but I wanted to propose/discuss the change first.

fluent-bit allows two JSON log formats: a stream of JSON objects not separated at all,
and one with a single JSON object per line. None of those was compatible with
grok_exporter so far. This change implements the json_lines format that is compatible
with fluent-bit.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 66.948% when pulling 5cb8fc1 on JensErat:json_lines into 44b195e on fstab:master.

@Skeen
Copy link
Contributor

Skeen commented May 17, 2020

I assume this PR is implementing support for Newline Delimited JSON (ndjson).

I think we could consider implementing a webhook_format: auto or webhook_format: detect for automatically determining the format, based upon the given Content-Type, in this case that would be application/x-ndjson.

  • I am not suggesting that it should be done with this PR, I'm just making sure that the format here is indeed ndjson.

@JensErat
Copy link
Contributor Author

Yes, looks like. Although fluent-bit not really calls it that way. fluent-bit allows sending custom headers, so using the header should also be a valid route for us. I'll check next week how fluent-bit really behaves regarding content type headers. I assume a way to "force overwrite" any incoming headers in grok_exporter would be a good thing, anyway.

I'm totally fine with providing a new patch taking HTTP headers into account.

I didn't know or even expect somebody wrote a spec for newline separated json records. :) But a common naming definitely is a good thing to have.

@Skeen
Copy link
Contributor

Skeen commented May 17, 2020

JensErat wrote:
Yes, looks like. Although fluent-bit not really calls it that way. fluent-bit allows sending custom headers, so using the header should also be a valid route for us. I'll check next week how fluent-bit really behaves regarding content type headers. I assume a way to "force overwrite" any incoming headers in grok_exporter would be a good thing, anyway.

I don't know the specifics of the fluent-bit http module, but I imagine it behaves somewhat similar to the fluentd one; which seems to send the header for ndjson, as quoted below:

FluentD documentation:
Content-Type for HTTP request. out_http automatically set Content-Type for built-in formatters when this parameter is not specified. Here is a table:

  • json: application/x-ndjson
  • ...

But yes, I'm not suggesting that auto / detect replaces a pre-configured value, rather that it might be a reasonable default.

I didn't know or even expect somebody wrote a spec for newline separated json records. :) But a common naming definitely is a good thing to have.

Ofcourse someone wrote a spec for it, it's always nice when things are standardized.

@Skeen
Copy link
Contributor

Skeen commented Aug 4, 2020

I have been running this change for a while now, and it seems good and rock stable. 👍

@fstab fstab merged commit f02f9c2 into fstab:master Sep 7, 2020
@fstab
Copy link
Owner

fstab commented Sep 7, 2020

Thanks a lot and sorry for the delay. It's merged now.

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.

4 participants