golang: allow accessing {request,response}/{headers,trailer} in the OnLog phase #34810

spacewander · 2024-06-19T07:54:13Z

Commit Message: allow accessing {request,response}/{headers,trailer} in the OnLog phase
Additional Description:
Risk Level: Low
Testing: Integration
Docs Changes:
Release Notes: Done
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2024-06-19T07:54:18Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34810 was opened by spacewander.

see: more, trace.

spacewander · 2024-06-19T16:13:17Z

CC @doujiang24
Please take a look.

doujiang24

There is a related issute #33873 to get headers at any stage, but I'm not against to get header in log phase for now.

doujiang24 · 2024-06-21T02:55:07Z

contrib/golang/filters/http/source/golang_filter.cc

-    is_golang_processing_log_ = true;
-    dynamic_lib_->envoyGoFilterOnHttpLog(req_, int(log_context.accessLogType()));
-    is_golang_processing_log_ = false;
+    decoding_state_.setFilterState(FilterState::ProcessingLog);


we'd better not overwrite the state?
Processing Log and Process DecodeXXX/EncodeXXX may run concurrently in Golang side.
use another flag, is_golang_processing_log_, could be much clear, thoughts?

@doujiang24
What about adding a pair of new methods:

setProcessingLog

isProcessingLog

So we can check if we are processing Go in the instance of ProcessorState.

Do you mean still write the logging state into decoding/encoding_state?
I don't think it is a good idea, logging state does not belongs to decoding/encoding, it would be some kind of hacky and make the code a bit complicated.
A new flag could be much clear.

doujiang24 · 2024-06-21T11:50:12Z

contrib/golang/filters/http/source/go/pkg/http/shim.go

+		},
+	}
+
+	var reqTrailer *requestTrailerMapImpl


Suggested change

var reqTrailer *requestTrailerMapImpl

var reqTrailer RequestTrailerMap

it will be nil as default value, so that we do not need those lines start at line 354?

@doujiang24
Thanks, updated.

doujiang24 · 2024-06-21T12:05:41Z

contrib/golang/filters/http/source/go/pkg/http/shim.go

@@ -276,16 +284,97 @@ func envoyGoFilterOnHttpLog(r *C.httpRequest, logType uint64) {

 	v := api.AccessLogType(logType)

+	// Request headers must exist because the HTTP filter won't be run if the headers are
+	// not sent yet.
+	reqHeader := &requestHeaderMapImpl{


better add a TODO commet: it's better to make it readonly?

tyxia · 2024-06-21T18:31:46Z

/wait

spacewander · 2024-06-24T02:23:53Z

There is a related issute #33873 to get headers at any stage, but I'm not against to get header in log phase for now.

Getting headers at any stage is well but it requires an extra event loop to fetch the headers from the Envoy, which will add ~100 microseconds. So using it in the per-request hot path is not a good choice.

spacewander · 2024-06-24T03:15:09Z

There is a related issute #33873 to get headers at any stage, but I'm not against to get header in log phase for now.

Getting headers at any stage is well but it requires an extra event loop to fetch the headers from the Envoy, which will add ~100 microseconds. So using it in the per-request hot path is not a good choice.

Getting request info in the log phase is a common requirement in API Gateway, for example, nearly 15% plugins of APISIX use this feature. So it would be better to avoid ~200 microseconds of extra latency (from both req and resp headers).

spacewander · 2024-06-24T05:43:50Z

There is a related issute #33873 to get headers at any stage, but I'm not against to get header in log phase for now.

Getting headers at any stage is well but it requires an extra event loop to fetch the headers from the Envoy, which will add ~100 microseconds. So using it in the per-request hot path is not a good choice.

Forget this, it's my mistake. Getting headers in the log phase doesn't require an extra event loop because the OnLog can't be run nonblockingly.

doujiang24 · 2024-06-24T11:04:33Z

contrib/golang/filters/http/source/go/pkg/http/shim.go

 	case api.AccessLogDownstreamEnd:
-		f.OnLog()
+		f.OnLog(reqHeader, reqTrailer, respHeader, respTrailer)


How about use the Envoy C++ API style:

OnLog(logContext) OnLogDownstreamPeriodic(logContext) OnLogDownstreamStart(logContext) type logContext interface { requestHeaders() RequestHeaderMap ... }

benifits:

we do not need to pre create four interface values even people won't use it. (we do only need one instead)

API fexible, we can add more APIs into logContext in the feature.

@doujiang24
I wonder how to connect this with the goal in #33873.
If we add the methods in the logContext, then we will have two different implementations to get request headers:
one can only use in OnLog phase and the other is used in the Encode phase.

This is just API style suggestion, no relation to #33873.
The implementations could keep the same as now, just user face API tweaks.

@doujiang24
It's not just user face API tweaks. It also affects how we implement it internally. If we bind the requestHeaders to the logContext, then we need to get the header from the logContext, which is unavailable in Encode phases. However, if we decide to support RequestHeaders in StreamInfo, and now we have two FilterCallbacks, we will probably fetch the headers from the XXFilterCallback directly.

This is my thought:

save the all of the parameters into logContext, i.e. decodingState, encodingState ...

create a RequestHeaderMap in logContext.requestHeaders, and etc.

we do not need any C/cgo related changes, just Go code tweaks, thoughts?

github-actions · 2024-07-26T12:01:25Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

spacewander · 2024-07-29T03:43:34Z

@doujiang24
What about creating new decodingState and encodingState in logContext?

We could not copy the States simply, as we may run both the encode header or something similar and the log phase, it could be not very clear to share the same state as they may affect each other. For example, they should use different phases to indicate which one is called.

doujiang24

otherwise, lgtm

doujiang24 · 2024-08-02T15:38:31Z

contrib/golang/filters/http/test/test_data/access_log/filter.go

+		return
+	}
+
+	reqHeader.Set("a", "b")


we'd better not write header in test? we comment they will be readonly: https://github.com/envoyproxy/envoy/pull/34810/files#r1648874832

doujiang24 · 2024-08-02T15:40:37Z

contrib/golang/filters/http/source/golang_filter.cc

+  uint64_t resp_trailer_num = 0;
+  uint64_t resp_trailer_bytes = 0;
+
+  auto decoding_s = dynamic_cast<processState*>(&decoding_state_);


style: decoding_state could be better.

doujiang24 · 2024-08-02T15:41:19Z

contrib/golang/filters/http/source/go/pkg/http/shim.go

@@ -266,7 +266,13 @@ func envoyGoFilterOnHttpData(s *C.processState, endStream, buffer, length uint64
 }

 //export envoyGoFilterOnHttpLog
-func envoyGoFilterOnHttpLog(r *C.httpRequest, logType uint64) {
+func envoyGoFilterOnHttpLog(r *C.httpRequest, logType uint64,
+	decodingS *C.processState, encodingS *C.processState,


style: decodingState could be better.

doujiang24 · 2024-08-14T01:34:44Z

/retest

doujiang24 · 2024-08-14T01:35:44Z

@spacewander The DCO failure should be fixed, https://github.com/envoyproxy/envoy/pull/34810/checks?check_run_id=28445721402

doujiang24

lgtm

…nLog phase Signed-off-by: spacewander <[email protected]>

spacewander · 2024-08-14T02:55:47Z

@doujiang24
Sorry for the push -f. I have to use this way to solve the DCO failure.

spacewander · 2024-08-14T06:00:36Z

/retest

spacewander · 2024-08-14T07:18:17Z

The failed test, TcpTunnelingIntegrationTest.UpstreamHttpFiltersPauseAndResume, is not caused by this change:
https://dev.azure.com/cncf/envoy/_build/results?buildId=177836&view=logs&jobId=8bf29878-a4cc-50f7-4e84-2255e6fd4065&j=8bf29878-a4cc-50f7-4e84-2255e6fd4065&t=150a3762-112b-5713-0d28-af1eb6c2fbf4

doujiang24 · 2024-08-14T10:54:32Z

/retest

…nLog phase (envoyproxy#34810) Signed-off-by: spacewander <[email protected]> Signed-off-by: asingh-g <[email protected]>

…nLog phase (envoyproxy#34810) Signed-off-by: spacewander <[email protected]>

…nLog phase (envoyproxy#34810) Signed-off-by: spacewander <[email protected]> Signed-off-by: duxin40 <[email protected]>

spacewander force-pushed the gocc branch from 97061f4 to 04d54e7 Compare June 19, 2024 08:21

spacewander marked this pull request as ready for review June 19, 2024 11:19

wbpcode assigned wangfakang and doujiang24 and unassigned wangfakang Jun 20, 2024

doujiang24 reviewed Jun 21, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 21, 2024

repokitteh-read-only bot removed the waiting label Jun 24, 2024

doujiang24 reviewed Jun 24, 2024

View reviewed changes

mattklein123 self-assigned this Jun 25, 2024

mattklein123 added the waiting label Jun 25, 2024

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 26, 2024

repokitteh-read-only bot removed the waiting label Jul 29, 2024

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 29, 2024

mattklein123 added the waiting label Jul 29, 2024

repokitteh-read-only bot removed the waiting label Aug 2, 2024

doujiang24 reviewed Aug 2, 2024

View reviewed changes

mattklein123 added the waiting label Aug 5, 2024

repokitteh-read-only bot removed the waiting label Aug 6, 2024

mattklein123 added the waiting label Aug 7, 2024

doujiang24 approved these changes Aug 14, 2024

View reviewed changes

golang: allow accessing {request,response}/{headers,trailer} in the O…

15c1317

…nLog phase Signed-off-by: spacewander <[email protected]>

spacewander force-pushed the gocc branch from da16ba2 to 15c1317 Compare August 14, 2024 02:52

repokitteh-read-only bot removed the waiting label Aug 14, 2024

Merge branch 'main' into gocc

ddce7a9

mattklein123 approved these changes Aug 14, 2024

View reviewed changes

mattklein123 merged commit 69ee8cf into envoyproxy:main Aug 14, 2024
47 checks passed

spacewander added a commit to mosn/envoy that referenced this pull request Sep 6, 2024

golang: allow accessing {request,response}/{headers,trailer} in the O…

c3be3f4

…nLog phase (envoyproxy#34810) Signed-off-by: spacewander <[email protected]>

ardikabs mentioned this pull request Sep 29, 2024

Support full header context with read-only on encode phase #33873

Open

duxin40 pushed a commit to duxin40/envoy that referenced this pull request Oct 15, 2024

golang: allow accessing {request,response}/{headers,trailer} in the O…

56dfdfe

…nLog phase (envoyproxy#34810) Signed-off-by: spacewander <[email protected]> Signed-off-by: duxin40 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

golang: allow accessing {request,response}/{headers,trailer} in the OnLog phase #34810

golang: allow accessing {request,response}/{headers,trailer} in the OnLog phase #34810

spacewander commented Jun 19, 2024

repokitteh-read-only bot commented Jun 19, 2024

spacewander commented Jun 19, 2024

doujiang24 left a comment

doujiang24 Jun 21, 2024

spacewander Jun 24, 2024

doujiang24 Jun 24, 2024

doujiang24 Jun 21, 2024

spacewander Jun 24, 2024

doujiang24 Jun 21, 2024

spacewander Jun 24, 2024

tyxia commented Jun 21, 2024

spacewander commented Jun 24, 2024

spacewander commented Jun 24, 2024

spacewander commented Jun 24, 2024

doujiang24 Jun 24, 2024

spacewander Jun 25, 2024

doujiang24 Jun 25, 2024

spacewander Jun 26, 2024

doujiang24 Jun 26, 2024

github-actions bot commented Jul 26, 2024

spacewander commented Jul 29, 2024

doujiang24 left a comment

doujiang24 Aug 2, 2024

doujiang24 Aug 2, 2024

doujiang24 Aug 2, 2024

doujiang24 commented Aug 14, 2024

doujiang24 commented Aug 14, 2024

doujiang24 left a comment

spacewander commented Aug 14, 2024

spacewander commented Aug 14, 2024

spacewander commented Aug 14, 2024

doujiang24 commented Aug 14, 2024

	var reqTrailer *requestTrailerMapImpl
	var reqTrailer RequestTrailerMap

golang: allow accessing {request,response}/{headers,trailer} in the OnLog phase #34810

golang: allow accessing {request,response}/{headers,trailer} in the OnLog phase #34810

Conversation

spacewander commented Jun 19, 2024

repokitteh-read-only bot commented Jun 19, 2024

spacewander commented Jun 19, 2024

doujiang24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxia commented Jun 21, 2024

spacewander commented Jun 24, 2024

spacewander commented Jun 24, 2024

spacewander commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 26, 2024

spacewander commented Jul 29, 2024

doujiang24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doujiang24 commented Aug 14, 2024

doujiang24 commented Aug 14, 2024

doujiang24 left a comment

Choose a reason for hiding this comment

spacewander commented Aug 14, 2024

spacewander commented Aug 14, 2024

spacewander commented Aug 14, 2024

doujiang24 commented Aug 14, 2024