Gin instrumentation: support reading Client IP from custom headers, and make sure proxy is trusted by ItalyPaleAle · Pull Request #6095 · open-telemetry/opentelemetry-go-contrib

ItalyPaleAle · 2024-09-08T21:34:46Z

With Gin, it's possible to configure the server to read the Client IP from custom headers; examples include X-Real-Ip or CF-Connecting-IP. This PR makes it possible to set as span attribute the same IP that Gin reads.

Additionally, it makes sure that headers such as "X-Forwarded-For" are used only if Gin is configured to trust the upstream server

linux-foundation-easycla · 2024-09-08T21:34:50Z

The committers listed above are authorized under a signed CLA.

✅ login: ItalyPaleAle / name: Alessandro (Ale) Segala (5236a72, 31c7ea0, e1c00de, 33dd2c8, 8a45c9d, 689f2c9, b897983, 69d16d0, 91ecb8e, 5739da4)
✅ login: dmathieu / name: Damien Mathieu (b19ba59)

dmathieu · 2024-09-09T07:02:45Z

PS: Also fixed unit tests, where there were assertions inside handlers, which are executed in separate goroutines

Could you split that into another PR, to make things easier to review?

ItalyPaleAle · 2024-09-09T13:59:24Z

PS: Also fixed unit tests, where there were assertions inside handlers, which are executed in separate goroutines

Could you split that into another PR, to make things easier to review?

Done

codecov · 2024-09-10T07:53:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.8%. Comparing base (1883203) to head (b19ba59).
Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #6095   +/-   ##
=====================================
  Coverage   75.7%   75.8%           
=====================================
  Files        207     207           
  Lines      19403   19469   +66     
=====================================
+ Hits       14695   14758   +63     
- Misses      4273    4275    +2     
- Partials     435     436    +1

Files with missing lines	Coverage Δ
...lei/go-restful/otelrestful/internal/semconv/env.go	`89.1% <100.0%> (ø)`
...o-restful/otelrestful/internal/semconv/httpconv.go	`86.9% <100.0%> (+0.1%)`	⬆️
...go-restful/otelrestful/internal/semconv/v1.20.0.go	`89.9% <100.0%> (ø)`
...stful/otelrestful/internal/semconvutil/httpconv.go	`95.7% <100.0%> (+<0.1%)`	⬆️
...hub.com/emicklei/go-restful/otelrestful/restful.go	`100.0% <100.0%> (ø)`
...umentation/github.com/gin-gonic/gin/otelgin/gin.go	`92.9% <100.0%> (+0.3%)`	⬆️
....com/gin-gonic/gin/otelgin/internal/semconv/env.go	`92.5% <100.0%> (ø)`
...gin-gonic/gin/otelgin/internal/semconv/httpconv.go	`86.9% <100.0%> (+0.1%)`	⬆️
.../gin-gonic/gin/otelgin/internal/semconv/v1.20.0.go	`89.9% <100.0%> (ø)`
...gonic/gin/otelgin/internal/semconvutil/httpconv.go	`95.7% <100.0%> (+<0.1%)`	⬆️
... and 17 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dmathieu

This will need a changelog entry.

ItalyPaleAle · 2024-09-29T02:37:09Z

@dmathieu is this PR ready to be merged?

dmathieu · 2024-09-29T12:44:47Z

PRs need two approvals.

ItalyPaleAle · 2024-10-07T02:35:16Z

PRs need two approvals.

@dmathieu is there a way I get a review/approval from another person?

dmathieu · 2024-10-07T07:26:45Z

Ping @open-telemetry/go-approvers for review.

Note that this instrumentation has no owner, and is therefore in the process of being deprecated/removed unless someone steps up to own it (which also explains the lack of reviews).
#6190

ItalyPaleAle · 2025-02-07T04:44:58Z

Looks like this instrumentation may have an owner now. If I fix the merge conflicts, can this PR be merged now?
@open-telemetry/go-approvers @akats7

akats7 · 2025-02-10T14:55:18Z

Hey @ItalyPaleAle, can you please resolve the conflicts

… sure proxy is trusted) With Gin, it's possible to configure the server to read the Client IP from custom headers; examples include `X-Real-Ip` or `CF-Connecting-IP`. This PR makes it possible to set as span attribute the same IP that Gin reads. Additionally, it makes sure that headers such as "X-Forwarded-For" are used only if Gin is configured to trust the upstream server PS: Also fixed unit tests, where there were assertions inside handlers, which are executed in separate goroutines

ItalyPaleAle · 2025-03-18T04:05:45Z

@akats7 sorry for the delay, as the amount of merge conflicts that had accumulated meant I had to recreate the PR from scratch.

Should be good for your review now!

ItalyPaleAle · 2025-03-22T01:39:50Z

Could I get a review on this please?

Also, is there a way to ensure PRs like this don't keep accumulating conflicts in the changelog?

…y-go-contrib into gin-clientip

ItalyPaleAle requested a review from hanyuancheung as a code owner September 8, 2024 21:34

ItalyPaleAle requested a review from a team September 8, 2024 21:34

dmathieu reviewed Sep 10, 2024

View reviewed changes

Comment thread instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go Outdated

ItalyPaleAle requested a review from dashpole as a code owner September 10, 2024 14:41

dmathieu reviewed Sep 11, 2024

View reviewed changes

dmathieu reviewed Sep 17, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread ...rumentation/github.com/emicklei/go-restful/otelrestful/internal/semconvutil/httpconv_test.go

dmathieu approved these changes Sep 18, 2024

View reviewed changes

dmathieu requested changes Sep 18, 2024

View reviewed changes

Comment thread instrumentation/github.com/gin-gonic/gin/otelgin/gintrace_test.go Outdated

dmathieu approved these changes Sep 20, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

ItalyPaleAle requested a review from a team as a code owner September 20, 2024 14:51

ItalyPaleAle commented Oct 7, 2024

View reviewed changes

Comment thread CHANGELOG.md Outdated

dmathieu requested a review from akats7 February 7, 2025 08:24

ItalyPaleAle added 2 commits March 17, 2025 21:04

Update changelog

5739da4

ItalyPaleAle force-pushed the gin-clientip branch from 666002a to 5739da4 Compare March 18, 2025 04:05

github-actions Bot requested a review from scorpionknifes March 18, 2025 04:05

dmathieu reviewed Mar 18, 2025

View reviewed changes

Comment thread instrumentation/github.com/emicklei/go-restful/otelrestful/restful.go

ItalyPaleAle added 2 commits March 18, 2025 22:24

Update semconv too

689f2c9

More things needed update

b897983

ItalyPaleAle added 2 commits March 18, 2025 22:41

Forgot the key part of this PR!

33dd2c8

Merge branch 'main' into gin-clientip

8a45c9d

dmathieu changed the title ~~Gin exporter: support reading Client IP from custom headers (and make sure proxy is trusted)~~ Gin instrumentation: support reading Client IP from custom headers (and make sure proxy is trusted Mar 19, 2025

ItalyPaleAle changed the title ~~Gin instrumentation: support reading Client IP from custom headers (and make sure proxy is trusted~~ Gin instrumentation: support reading Client IP from custom headers, and make sure proxy is trusted Mar 19, 2025

Merge branch 'main' into gin-clientip

e1c00de

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

31c7ea0

…y-go-contrib into gin-clientip

akats7 reviewed Mar 24, 2025

View reviewed changes

Comment thread instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go

dmathieu reviewed Mar 24, 2025

View reviewed changes

Comment thread CHANGELOG.md Outdated

Comment thread instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go

ItalyPaleAle added 2 commits March 24, 2025 08:55

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

91ecb8e

…y-go-contrib into gin-clientip

Move changelog

69d16d0

akats7 approved these changes Mar 24, 2025

View reviewed changes

Merge branch 'main' into gin-clientip

b19ba59

dmathieu merged commit a262a05 into open-telemetry:main Mar 25, 2025

ItalyPaleAle deleted the gin-clientip branch March 25, 2025 14:51

MrAlias added this to the v1.36.0 milestone Mar 27, 2025

dmathieu mentioned this pull request May 21, 2025

Release v1.36.0/v0.61.0/v0.30.0/v0.16.0/v0.11.0/v0.9.0/v0.8.0 #7350

Merged

Conversation

ItalyPaleAle commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmathieu commented Sep 9, 2024

Uh oh!

ItalyPaleAle commented Sep 9, 2024

Uh oh!

Uh oh!

codecov Bot commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmathieu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ItalyPaleAle commented Sep 29, 2024

Uh oh!

dmathieu commented Sep 29, 2024

Uh oh!

Uh oh!

ItalyPaleAle commented Oct 7, 2024

Uh oh!

dmathieu commented Oct 7, 2024

Uh oh!

ItalyPaleAle commented Feb 7, 2025

Uh oh!

akats7 commented Feb 10, 2025

Uh oh!

ItalyPaleAle commented Mar 18, 2025

Uh oh!

Uh oh!

ItalyPaleAle commented Mar 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ItalyPaleAle commented Sep 8, 2024 •

edited

Loading

linux-foundation-easycla Bot commented Sep 8, 2024 •

edited

Loading

codecov Bot commented Sep 10, 2024 •

edited

Loading