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 mode to Loki client #36

Merged
merged 1 commit into from
Feb 5, 2022

Conversation

juliusrickert
Copy link
Contributor

I experienced problems with newlines in the text-based logging. This motivated me to add a simple JSON mode to the Loki client.

@dmachard
Copy link
Owner

dmachard commented Feb 4, 2022

Thanks for this PR.
Can you share some details regarding your issue with newlines ?

@juliusrickert
Copy link
Contributor Author

Je vous en prie !
Thank you for developing and providing this piece of software.

First, a minor issue: the log lines end in \n, requiring the pattern to match it to end in \n.

But furthermore, I'm seeing domain names starting in "-n " (including the space), having newlines in them, etc. This is breaking my Loki pattern.
I'm not sure what the origin of this behaviour is (yet).

In general, I think it's dangerous to use client-provided data without escaping it (:eyes: Log4Shell). Thus, I think a JSON-based logger for Loki is beneficial.

@dmachard
Copy link
Owner

dmachard commented Feb 4, 2022

Could you share an example of a full line?
I haven't observed any issues on my side with text mode

@juliusrickert
Copy link
Contributor Author

I think I can share a redacted example, but on Monday at the earliest, as I don't have access from my personal computer.

But I think that's kind of unrelated to adding this feature. What do you think about merging it?

@dmachard
Copy link
Owner

dmachard commented Feb 5, 2022

I just want to understand what happened in your case and why I haven't observed that in my side.

@dmachard dmachard merged commit 5e0e439 into dmachard:main Feb 5, 2022
@juliusrickert
Copy link
Contributor Author

No problem. Thank you for merging.

I'll open an issue about this if I have any news.

@dmachard
Copy link
Owner

dmachard commented Feb 6, 2022

Thank, You can also open a discussion regarding Loki with the DNS-Collector; it will be great to share implementation.

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.

2 participants