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(tracing): add http.client_ip attribute #10723

Merged
merged 8 commits into from
Apr 28, 2023
Merged

Conversation

backjo
Copy link
Contributor

@backjo backjo commented Apr 23, 2023

Summary

As a developer troubleshooting requests using trace data, I expect my access logs to be consistent with my traces. The access logs currently record the forwarded_ip, which tells me what my client's true ip is (assuming proper setup of trusted IPs etc). This change adds a span attribute for client ip using the forwarded ip

Checklist

Full changelog

  • [Implement ...]

Issue reference

Fix #[issue number]

As a developer troubleshooting requests using trace data, I expect my access logs to be consistent with my traces. The access logs currently record the forwarded_ip, which tells me what my client's true ip is (assuming proper setup of trusted IPs etc). This change switches the span attribute for peer ip to use forwarded_ip as well.
@backjo backjo changed the title fix(tracing): use get_forwarded_ip instead of get_ip for peer IP feat(tracing): add http.client_ip attribute Apr 24, 2023
@oowl
Copy link
Member

oowl commented Apr 25, 2023

@backjo Could you help us to fix the test? This is related to your feature. https://github.com/Kong/kong/blob/master/spec/02-integration/14-tracing/01-instrumentations_spec.lua#L138

@backjo
Copy link
Contributor Author

backjo commented Apr 25, 2023

@attenuation will update this PR after the earlier

@backjo Could you help us to fix the test? This is related to your feature. https://github.com/Kong/kong/blob/master/spec/02-integration/14-tracing/01-instrumentations_spec.lua#L138

Should be fixed - added test attribute to verify this new one

@StarlightIbuki
Copy link
Contributor

StarlightIbuki commented Apr 26, 2023

Thanks for your contribution. The change looks good. Please add a proper changelog.

Copy link
Member

@oowl oowl left a comment

Choose a reason for hiding this comment

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

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#http-client,

The IP address of the original client behind all proxies, if known (e.g. from X-Forwarded-For). [2]

That means:
http.client_ip is the real client IP
net.peer.ip is the proxy socket source IP.

@backjo
Copy link
Contributor Author

backjo commented Apr 27, 2023

Added the changelog!

@hanshuebner hanshuebner merged commit 57b9ae6 into Kong:master Apr 28, 2023
locao added a commit that referenced this pull request May 19, 2023
locao added a commit that referenced this pull request May 22, 2023
team-gateway-bot pushed a commit that referenced this pull request May 22, 2023
locao added a commit that referenced this pull request May 22, 2023
(cherry picked from commit 354a302)

Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants