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

feat(stdlib): Add ingress_upstreaminfo log format to parse_nginx_log function #193

Merged
merged 4 commits into from
May 24, 2023

Conversation

esergion
Copy link
Contributor

Fixes #5

Also, what do you guys think on renaming parsed fields like they are named in Nginx docs here?

I would stick with that nginx's internal fields naming "convention", it's so close to RFCs at least.

If you are agree with this, I would make another PR to rename fields in "combined" format too

@bruceg bruceg requested a review from a team April 19, 2023 16:59
@bruceg bruceg added type: enhancement A value-adding code change that enhances its existing functionality vrl: stdlib Changes to the standard library labels Apr 19, 2023
Copy link
Member

@fuchsnj fuchsnj left a comment

Choose a reason for hiding this comment

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

This mostly looks good. Just a few changes requested to the type definition.

Also, please add a line to the CHANGELOG.md with a summary of these changes.

lib/stdlib/src/parse_nginx_log.rs Outdated Show resolved Hide resolved
lib/stdlib/src/parse_nginx_log.rs Outdated Show resolved Hide resolved
lib/stdlib/src/parse_nginx_log.rs Outdated Show resolved Hide resolved
lib/stdlib/src/parse_nginx_log.rs Outdated Show resolved Hide resolved
lib/stdlib/src/parse_nginx_log.rs Outdated Show resolved Hide resolved
lib/stdlib/src/parse_nginx_log.rs Outdated Show resolved Hide resolved
@fuchsnj
Copy link
Member

fuchsnj commented May 10, 2023

Also, what do you guys think on renaming parsed fields like they are named in Nginx docs here?

That seems reasonable to me, although this would be a breaking change. Curious if anyone else has thoughts here. I think some field names were chosen initially to match Vector conventions (such as timestamp), but I think following those conventions will be less important in the future.

@spencergilbert
Copy link
Contributor

That seems reasonable to me, although this would be a breaking change. Curious if anyone else has thoughts here. I think some field names were chosen initially to match Vector conventions (such as timestamp), but I think following those conventions will be less important in the future.

I'd agree with matching the "upstream" naming schemes 👍

@esergion esergion force-pushed the ingress-nginx-log-format branch 3 times, most recently from 7d0c2f7 to e2b7103 Compare May 11, 2023 10:57
@esergion
Copy link
Contributor Author

Thanks for the review, pushed all the requested changes

@jszwedko jszwedko enabled auto-merge (squash) May 18, 2023 15:44
auto-merge was automatically disabled May 22, 2023 17:06

Base branch was modified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A value-adding code change that enhances its existing functionality vrl: stdlib Changes to the standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VRL: Add ingress-nginx log format for parse_nginx_log
4 participants