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 key value http logger #358

Merged

Conversation

nsmolenskii
Copy link
Contributor

@nsmolenskii nsmolenskii commented Oct 7, 2018

Description

Add support key value http formatters

Motivation and Context

Some log collecting software (like Splunk), use key value logging formats by default.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

⚠️ depends on #357


import static org.apiguardian.api.API.Status.EXPERIMENTAL;

public abstract class AbstractPreparedHttpLogFormatter implements HttpLogFormatter {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be an interface with default methods, couldn't it?

import java.util.Map;
import java.util.stream.Collectors;

public class KeyValueHttpLogFormatter extends AbstractPreparedHttpLogFormatter {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format is rather specific to splunk, why not call it SplunkHttpLogFormatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, splunk just uses key-value pairs, but it does not mean that every key-value format consumer is splunk.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but until someone comes along who needs something similar outside of splunk I'd prefer to be specific. That gives us a bit more freedom in evolving this along with splunk, if needed.

@Override
public String format(final Map<String, Object> content) {
return content.entrySet().stream()
.map(entry -> String.format("%s=%s", entry.getKey(), entry.getValue()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any escaping that needs to be done for values that contain a = e.g. the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that is a idiomatic question.
Splunk for example is eating such format like uri=http://host?k=v and allows you to search it. But also during the syntax highlighting, it suggest two pairs uri=http://host and k=v, which allow you to search those parameters bound to each other (full search) or independently (partial search). It is quite useful, when you have many parameters in uri, but you is interested in only some.

@nsmolenskii nsmolenskii mentioned this pull request Oct 8, 2018
6 tasks
Copy link
Collaborator

@whiskeysierra whiskeysierra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to rename kv to splunk and the KeyValueHttpLogFormatter to SplunkHttpLogFormatter.

@whiskeysierra
Copy link
Collaborator

Can you resolve the conflict?

@whiskeysierra
Copy link
Collaborator

Can you resolve the conflict?

Nevermind. I just saw that Github has a web editor for this now. Let's see whether it worked.

@whiskeysierra
Copy link
Collaborator

Not really. Looks like the whole README is now somehow modified.

@zalando zalando deleted a comment Oct 11, 2018
@nsmolenskii nsmolenskii force-pushed the add-key-value-http-formatter branch from ef88432 to ad6a3c0 Compare October 11, 2018 05:57
@nsmolenskii
Copy link
Contributor Author

Not really. Looks like the whole README is now somehow modified.

Yeah 😞. I had rebase my branch against master - looks better now.

@whiskeysierra whiskeysierra merged commit b053e8f into zalando:master Oct 11, 2018
@whiskeysierra
Copy link
Collaborator

Thanks a lot for the contribution! 🎉 I'll prepare a new release as soon as I get to the office.

@nsmolenskii nsmolenskii deleted the add-key-value-http-formatter branch October 11, 2018 06:11
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