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

adds host and path to structured log formats #916

Merged
merged 4 commits into from Jan 6, 2021

Conversation

nhomble
Copy link
Contributor

@nhomble nhomble commented Dec 25, 2020

Proposed change for #915

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.

@whiskeysierra
Copy link
Collaborator

whiskeysierra commented Dec 25, 2020

Thanks for your pull request! 📣

Looks good so far. Just one thing to add:
This change affects both, the JsonHttpLogFormatter and the SplunkHttpLogFormatter. The docs for both should be adjusted accordingly:

Another idea, while we're at it: What about scheme, port and query? If we break down the request uri into its components, why not apply it all the way?

@nhomble
Copy link
Contributor Author

nhomble commented Dec 25, 2020

Sorry for missing the docs, I'll definitely update.

I also agree that it makes sense to add all of the components. One question, should we be consistent with the prepareHeaders/prepareBody pattern and ignore port when optional is empty and ignore query when query is empty?

@whiskeysierra
Copy link
Collaborator

Good question. I already thought about it. I guess absent would be more consistent with the existing features. But null values or defaults could be easier to use on the log analytics site of things.

@nhomble
Copy link
Contributor Author

nhomble commented Dec 25, 2020

I think it would be reasonable to put null as a default port. And then if users want to remap to some numeric default they can do that in their query.
I'm also thinking for query, it would be nice to parse the map out of the string and insert into content similar to headers. Beyond making searching easier, I think if we make this a utility method of HttpRequest then the QueryFilter implementation could become easier.

- update json/splunk format readme docs
@nhomble
Copy link
Contributor Author

nhomble commented Dec 25, 2020

Since the other keys are pretty straightforward, I'm thinking we tackle the query params separate from the other simpler fields (happy to follow up with another pr). I was also a little confused by the readme since it mentioned path in both the json and splunk examples. If there is another transformation happening somewhere, I could use help seeing where that happens.

@whiskeysierra
Copy link
Collaborator

I'm also thinking for query, it would be nice to parse the map out of the string and insert into content similar to headers. Beyond making searching easier, I think if we make this a utility method of HttpRequest then the QueryFilter implementation could become easier.

Yes, that would be nice.

Since the other keys are pretty straightforward, I'm thinking we tackle the query params separate from the other simpler fields (happy to follow up with another pr).

Sounds good.

I was also a little confused by the readme since it mentioned path in both the json and splunk examples. If there is another transformation happening somewhere, I could use help seeing where that happens.

I guess it's just out of date with the implementation. Maybe I had this in mind at some point but never implemented it.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
nhomble and others added 2 commits January 4, 2021 20:19
Co-authored-by: Willi Schönborn <w.schoenborn@gmail.com>
@@ -385,7 +389,8 @@ requests and response as key-value pairs.
###### Request

```text
origin=remote type=request correlation=2d66e4bc-9a0d-11e5-a84c-1f39510f0d6b protocol=HTTP/1.1 sender=127.0.0.1 method=POST path=http://example.org/test headers={Accept=[application/json], Content-Type=[text/plain]} body=Hello world!
origin=remote type=request correlation=2d66e4bc-9a0d-11e5-a84c-1f39510f0d6b protocol=HTTP/1.1 sender=127.0.0.1 method=POST uri=http://example.org/test host=example.org scheme=http port=null path=/test headers={Accept=[application/json], Content-Type=[text/plain]} body=Hello world!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not an expert on splunk - maybe an empty value would be cleaner?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@whiskeysierra
Copy link
Collaborator

👍

@whiskeysierra whiskeysierra merged commit 982d970 into zalando:main Jan 6, 2021
@whiskeysierra
Copy link
Collaborator

Thanks for your contribution! 🎉

I'll wait for your other PRs to make one big release, unless you need it faster.

@nhomble
Copy link
Contributor Author

nhomble commented Jan 6, 2021

@whiskeysierra thanks! I think I should have bandwidth to finish our open prs and propose something for the query params.

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.

None yet

2 participants