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(kumacp) included traffic direction in access log #682

Merged
merged 3 commits into from
Apr 28, 2020

Conversation

tharun208
Copy link
Contributor

Summary

Included trafficDirection in the access log.

Full changelog

  1. defaultHttpAccessLogFormat
  2. defaultNetworkAccessLogFormat

Issues resolved

Fix #598

@tharun208 tharun208 requested a review from a team April 16, 2020 07:58
@tharun208 tharun208 marked this pull request as draft April 16, 2020 08:01
@tharun208 tharun208 marked this pull request as ready for review April 16, 2020 08:12
@subnetmarco
Copy link
Contributor

Thanks @tharun208 - Let us review this.

Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz 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 omit placing it in the default configuration. We always put the logging on outbound at the moment.

@tharun208
Copy link
Contributor Author

I'd omit to place it in the default configuration. We always put the logging on outbound at the moment.
should I need to remove

I'd omit placing it in the default configuration. We always put the logging on outbound at the moment.

I am not getting you on this. Can you explain a bit more, please?

@jakubdyszkiewicz
Copy link
Contributor

%KUMA_TRAFFIC_DIRECTION% indicates whether the traffic that we are logging is incoming or outgoing. The thing is that right now we only log outgoing traffic (placing logging filter in outbound listeners), therefore, I don't think this information is that usefull that we have to put it in the default format.

@tharun208
Copy link
Contributor Author

%KUMA_TRAFFIC_DIRECTION% indicates whether the traffic that we are logging is incoming or outgoing. The thing is that right now we only log outgoing traffic (placing logging filter in outbound listeners), therefore, I don't think this information is that useful that we have to put it in the default format.

yes! agreed, within our outbound and inbound listeners we are showing traffic direction, can i close this PR ?

@jakubdyszkiewicz
Copy link
Contributor

This functionality itself might be useful in the future. Just don't add it to defaultHttpAccessLogFormat and defaultNetworkAccessLogFormat.

@tharun208
Copy link
Contributor Author

This functionality itself might be useful in the future. Just don't add it to defaultHttpAccessLogFormat and defaultNetworkAccessLogFormat.

should we need this to be added to all the configurer within kuma?

@jakubdyszkiewicz
Copy link
Contributor

should we need this to be added to all the configurer within kuma?

I'm sorry, I don't think I get the question.
Everything about this PR is fine except change in default formats.

@tharun208
Copy link
Contributor Author

should we need this to be added to all the configurer within kuma?

I'm sorry, I don't think I get the question.
Everything about this PR is fine except change in default formats.

should I need to change it to Kuma direction to UNSPECIFIED?

@jakubdyszkiewicz
Copy link
Contributor

No, but this code

const defaultHttpAccessLogFormat = `[%START_TIME%] %KUMA_MESH% "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%KUMA_SOURCE_SERVICE%" "%KUMA_DESTINATION_SERVICE%" "%KUMA_SOURCE_ADDRESS_WITHOUT_PORT%" "%UPSTREAM_HOST%"	

const defaultNetworkAccessLogFormat = `[%START_TIME%] %RESPONSE_FLAGS% %KUMA_MESH% %KUMA_SOURCE_ADDRESS_WITHOUT_PORT%(%KUMA_SOURCE_SERVICE%)->%UPSTREAM_HOST%(%KUMA_DESTINATION_SERVICE%) took %DURATION%ms, sent %BYTES_SENT% bytes, received: %BYTES_RECEIVED% bytes	

should stay as is.

@jakubdyszkiewicz jakubdyszkiewicz merged commit ba5bbfe into kumahq:master Apr 28, 2020
@jakubdyszkiewicz
Copy link
Contributor

thanks!

@tharun208 tharun208 deleted the feat/#598 branch April 28, 2020 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants