Skip to content

feat: print matched error logs as json if requested #264

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fionera
Copy link
Contributor

@fionera fionera commented Aug 4, 2025

Previously the json handler would only wrap the message in a json object. This now replaces that message with an embedded object that contains all the information.

Thanks to @superstes for implementing this in his fork

Previously the json handler would only wrap the message in a json object. This now replaces that message with an embedded object that contains all the information.
@fionera fionera requested a review from fzipi August 4, 2025 10:48
@fzipi
Copy link
Member

fzipi commented Aug 4, 2025

Should we add tests?

@hedgieinsocks
Copy link

Maybe we could abc sort the keys in the code? Right now, errorLog struct and marshalling use different key orders, which creates a small visual confusion. As for tests, the whole project has only the e2e test, so maybe adding unit tests (if they make sense) could be done in a separate contribution?

@hedgieinsocks
Copy link

@fionera just poking you to push this through as I want to see 1) 0.4.0 released right after that with a semver image 2) be able to finally build grafana dashboard from elastic json fields :D

@fionera
Copy link
Contributor Author

fionera commented Aug 16, 2025

Sorting makes no sense as this is not targeted to be human readable. Tests would be something we have to add in general, so yeah I would just keep it as this is.

@hedgieinsocks
Copy link

hedgieinsocks commented Aug 17, 2025

I meant sorting keys or at least making them in identical order in the code to make it predictable and readable development-wise. Cause I wanted to raise a comment about a missing key but later saw it present in another place. Also, I think we are missing phase key here.

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