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

Observability: Access Logging #1407

Merged
merged 13 commits into from
May 26, 2023
Merged

Observability: Access Logging #1407

merged 13 commits into from
May 26, 2023

Conversation

zirain
Copy link
Contributor

@zirain zirain commented May 8, 2023

xref: #699

base on #1121, provide access log setting per EnvoyProxy

cc @arkodg @AliceProxy @kflynn @Xunzhuo

@zirain zirain added area/api API-related issues area/observability Observability related issues labels May 8, 2023
@zirain zirain requested a review from a team as a code owner May 8, 2023 06:26
@zirain zirain mentioned this pull request May 8, 2023
Closed
@codecov
Copy link

codecov bot commented May 8, 2023 •
edited
Loading

Codecov Report

Merging #1407 (4122002) into main (41c215f) will decrease coverage by 0.51%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1407      +/-   ##
==========================================
- Coverage   62.01%   61.50%   -0.51%     
==========================================
  Files          79       79              
  Lines       11388    11473      +85     
==========================================
- Hits         7062     7057       -5     
- Misses       3867     3956      +89     
- Partials      459      460       +1
Impacted Files Coverage Δ
api/config/v1alpha1/envoyproxy_types.go 100.00% <ø> (ø)
api/config/v1alpha1/zz_generated.deepcopy.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

kflynn
kflynn suggested changes May 9, 2023
View reviewed changes
Copy link
Contributor

@kflynn kflynn 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 vote to pull out filtering at first, for certain.

I'm a little torn on the logging API itself: being able to define multiple log mechanisms feels useful, but is it really? If we don't think it's actually useful, I'd probably vote for a simpler syntax.

@kflynn
Copy link
Contributor

kflynn commented May 9, 2023

@zirain Quick question: if you specify both text and fields for OpenTelemetry logging, does that send two requests to the OTel provider, or one?

@zirain
Copy link
Contributor Author

zirain commented May 10, 2023

@kflynn one request

after offline talk with @arkodg , will simply the design, will update later.

@zirain
Observability: Access Logging
6925214 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain
fix lint
28c1d1f 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain
update after discussion with arko
cbcf781 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain
Copy link
Contributor Author

zirain commented May 11, 2023

@kflynn sorry, can you take a look again?

@zirain zirain requested review from kflynn and arkodg May 12, 2023 02:22
@arkodg
Copy link
Contributor

arkodg commented May 12, 2023

thanks for updating the PR, +1 on the overall layout of fields, added some comments around field names

@zirain
update with suggestion
ceef6e8 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain
Merge branch 'main' into accesslogging
e35d206
@zirain
codecov ignore api folder
b9c52be 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
haq204
haq204 reviewed May 12, 2023
View reviewed changes
api/config/v1alpha1/envoyproxy_types.go Outdated Show resolved Hide resolved
api/config/v1alpha1/envoyproxy_types.go Outdated Show resolved Hide resolved
api/config/v1alpha1/envoyproxy_types.go Outdated Show resolved Hide resolved
@zirain
update
f5c022f 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain
reorg and use slice
3c502ac 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain zirain requested review from arkodg and haq204 May 23, 2023 01:22
arkodg
arkodg reviewed May 23, 2023
View reviewed changes
.github/codecov.yml Outdated Show resolved Hide resolved
@zirain
Merge branch 'main' into accesslogging
c6e59ec
@zirain
revert codecov
0dd4679 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain
update with arko's comment
e1de110 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain
Merge branch 'main' into accesslogging
98ce788
@zirain
rename Fields to JSON
4122002 
Signed-off-by: hejianpeng <hejianpeng2@huawei.com>
@zirain zirain requested a review from arkodg May 25, 2023 01:15
@zirain
Copy link
Contributor Author

zirain commented May 25, 2023

@arkodg @kflynn @haq204 PTAL

arkodg
arkodg approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

arkodg
arkodg reviewed May 25, 2023
View reviewed changes
docs/latest/design/accesslogging.md
// can be used as values for fields within the Struct.
// It's required when the format type is "json".
// +optional
Fields map[string]string `json:"fields,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be updated to reflect newer API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs to be updated to reflect newer API

will done it within a followup PR

kflynn
kflynn approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! When this starts getting implementations, we'll (of course) need tests, but for defining the API I think this is good to go. 🙂

@zirain
Copy link
Contributor Author

zirain commented May 26, 2023

Looks good, thanks! When this starts getting implementations, we'll (of course) need tests, but for defining the API I think this is good to go. 🙂

yes, we already setup e2e framework.

@zirain zirain merged commit b505963 into envoyproxy:main May 26, 2023
17 of 18 checks passed
@zirain zirain deleted the accesslogging branch May 26, 2023 00:41
@zirain zirain mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arkodg arkodg arkodg approved these changes

@kflynn kflynn kflynn approved these changes

@AliceProxy AliceProxy AliceProxy approved these changes

@haq204 haq204 Awaiting requested review from haq204

Assignees
No one assigned
Labels
area/api API-related issues area/observability Observability related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zirain @kflynn @arkodg @AliceProxy @haq204