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.

Sign up for GitHub

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

Jump to bottom

Add grpc_status_number as a default access log field #4880

Merged
merged 1 commit into from
Dec 16, 2022
Merged

Add grpc_status_number as a default access log field #4880

merged 1 commit into from
Dec 16, 2022

Conversation

rajatvig
Copy link
Member

Fixes #4484

Signed-off-by: Rajat Vig rvig@etsy.com

@rajatvig rajatvig requested a review from a team as a code owner November 30, 2022 11:45
@rajatvig rajatvig requested review from skriss and sunjayBhatia and removed request for a team November 30, 2022 11:45
@codecov
Copy link

codecov bot commented Nov 30, 2022 •
edited
Loading

Codecov Report

Merging #4880 (4d8c07a) into main (30320aa) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 4d8c07a differs from pull request most recent head 46b6900. Consider uploading reports for the commit 46b6900 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4880      +/-   ##
==========================================
- Coverage   76.45%   76.45%   -0.01%     
==========================================
  Files         140      140              
  Lines       16711    16711              
==========================================
- Hits        12777    12776       -1     
- Misses       3684     3685       +1     
  Partials      250      250
Impacted Files Coverage Δ
internal/sorter/sorter.go 98.46% <0.00%> (-0.52%) ⬇️

@rajatvig rajatvig added the release-note/small A small change that needs one line of explanation in the release notes. label Nov 30, 2022
@rajatvig
Copy link
Member Author

rajatvig commented Dec 8, 2022

Could this be reviewed please?

skriss
skriss requested changes Dec 15, 2022
View reviewed changes
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

One minor comment on the changelog, otherwise LGTM.

cc @sunjayBhatia

changelogs/unreleased/4880-rajatvig-small.md Outdated Show resolved Hide resolved
@skriss skriss changed the title Add grpc_status_code as a default access log field Add grpc_status_number as a default access log field Dec 15, 2022
sunjayBhatia
sunjayBhatia reviewed Dec 15, 2022
View reviewed changes
apis/projectcontour/v1alpha1/accesslog.go
@@ -43,6 +43,7 @@ var DefaultAccessLogJSONFields = AccessLogJSONFields([]string{
"user_agent",
"x_forwarded_for",
"grpc_status",
"grpc_status_number",
Copy link
Member

Choose a reason for hiding this comment

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

huh seems like we already added this to envoySimpleOperators in this file

Copy link
Member

Choose a reason for hiding this comment

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

(the access log format operator version)

sunjayBhatia
sunjayBhatia approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

might want to be more judicious about the default access log fields in the future, but seems good to add this one

@sunjayBhatia
Copy link
Member

might want to be more judicious about the default access log fields in the future, but seems good to add this one

lgtm minus @skriss' comments I should say

@rajatvig @skriss
Add grpc_status_code as a default access log field
46b6900 
Fixes projectcontour#4484

Signed-off-by: Rajat Vig <rvig@etsy.com>

Co-authored-by: Steve Kriss <stephen.kriss@gmail.com>
Signed-off-by: Rajat Vig <rvig@etsy.com>
@rajatvig
Copy link
Member Author

might want to be more judicious about the default access log fields in the future, but seems good to add this one

lgtm minus @skriss' comments I should say

Addressed the comments. Agree, the default access log fields should only be judiciously added.

@skriss
Copy link
Member

skriss commented Dec 16, 2022

Thanks @rajatvig!

@skriss skriss merged commit 0591fc2 into projectcontour:main Dec 16, 2022
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
@rajatvig @yangyy93
Add grpc_status_number as a default access log field (projectcontour#…
528b425 
…4880)

Fixes projectcontour#4484.

Signed-off-by: Rajat Vig <rvig@etsy.com>
Signed-off-by: yy <yang.yang@daocloud.io>
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
@rajatvig @yangyy93
Add grpc_status_number as a default access log field (projectcontour#…
8179f85 
…4880)

Fixes projectcontour#4484.

Signed-off-by: Rajat Vig <rvig@etsy.com>
Signed-off-by: yy <yang.yang@daocloud.io>
vmw-yingy pushed a commit to vmw-yingy/contour that referenced this pull request Feb 28, 2023
@rajatvig @vmw-yingy
Add grpc_status_number as a default access log field (projectcontour#…
8e8c92a 
…4880)

Fixes projectcontour#4484.

Signed-off-by: Rajat Vig <rvig@etsy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunjayBhatia sunjayBhatia sunjayBhatia approved these changes

@skriss skriss skriss approved these changes

Assignees
No one assigned
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add grpc_status_number to access logs
3 participants
@rajatvig @sunjayBhatia @skriss