open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.
https://opentelemetry.io/
Apache License 2.0
1.15k stars 544 forks source link

feat: Add *gin.Context Filter parameter #5743

Closed rehanpfmr closed 1 month ago

rehanpfmr commented 3 months ago

Issue: https://github.com/open-telemetry/opentelemetry-go-contrib/issues/3070 last PR status: https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4444

dmathieu commented 3 months ago

This PR seems unfinished (the GinFilter isn't used, the WithGinFilter method accepts a Filter, not a GinFilter and there are no tests). I'm moving it to draft. Feel free to undraft once you are finished.

rehanpfmr commented 3 months ago

@dmathieu Please review the changes, I have added the filter with test.

dmathieu commented 3 months ago

Did you run those tests? The filter exists, but isn't used anywhere. The code is also showing indentation issues that will fail linting.

Note : I'm not sure this is a good solution. @hanyuancheung, what do you think?

rehanpfmr commented 3 months ago

@dmathieu Please refer to https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4444, the solution was recommended here. I'm looking at the Test for "TestWithGinFilter" is currently failing.

rehanpfmr commented 3 months ago

@dmathieu, Can you please review? The test with 'make build' seems to be passing.

dmathieu commented 3 months ago

Since this is changing the public API, I would like @hanyuancheung's opinion as code owner.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.5%. Comparing base (a32ef13) to head (ee1dbbb). Report is 1 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/5743/graphs/tree.svg?width=650&height=150&src=pr&token=P6F3W9WA7Q&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/5743?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #5743 +/- ## ===================================== Coverage 65.5% 65.5% ===================================== Files 203 203 Lines 12918 12926 +8 ===================================== + Hits 8466 8474 +8 Misses 4198 4198 Partials 254 254 ``` | [Files](https://app.codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/5743?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [...ation/github.com/gin-gonic/gin/otelgin/gintrace.go](https://app.codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/5743?src=pr&el=tree&filepath=instrumentation%2Fgithub.com%2Fgin-gonic%2Fgin%2Fotelgin%2Fgintrace.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-aW5zdHJ1bWVudGF0aW9uL2dpdGh1Yi5jb20vZ2luLWdvbmljL2dpbi9vdGVsZ2luL2dpbnRyYWNlLmdv) | `82.1% <100.0%> (+1.0%)` | :arrow_up: | | [...ntation/github.com/gin-gonic/gin/otelgin/option.go](https://app.codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/5743?src=pr&el=tree&filepath=instrumentation%2Fgithub.com%2Fgin-gonic%2Fgin%2Fotelgin%2Foption.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-aW5zdHJ1bWVudGF0aW9uL2dpdGh1Yi5jb20vZ2luLWdvbmljL2dpbi9vdGVsZ2luL29wdGlvbi5nbw==) | `100.0% <100.0%> (ø)` | |
rehanpfmr commented 2 months ago

@dmathieu , Fixed the lint test. Hope this should be all good now.

rehanpfmr commented 2 months ago

@dmathieu , Based on the recent commit lint fix it should fix this. Please help run the test thanks

rehanpfmr commented 2 months ago

@dmathieu , Did you get chance to review the test please?

rehanpfmr commented 2 months ago

@dmathieu, Are we good to make an PR merge? Thanks

dmathieu commented 2 months ago

No. We need a second review.

rehanpfmr commented 1 month ago

@hanyuancheung, Please help approve this PR. Thanks

dmathieu commented 1 month ago

cc @open-telemetry/go-approvers

rehanpfmr commented 1 month ago

@open-telemetry/go-approvers @hanyuancheung Would you please help approve this PR, we need an second reviewer for the PR merge.

Thanks

hanyuancheung commented 1 month ago

Since this is changing the public API, I would like @hanyuancheung's opinion as code owner.

This PR's change LGTM!

rehanpfmr commented 1 month ago

Thanks @hanyuancheung for the approval. @dmathieu are we good to merge PR ?

rehanpfmr commented 1 month ago

@dmathieu, Thank you for all the review and feedback.