opentelemetry-go-contrib icon indicating copy to clipboard operation
opentelemetry-go-contrib copied to clipboard

feat: Add *gin.Context Filter parameter

Open rehanpfmr opened this issue 1 year ago • 7 comments

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

rehanpfmr avatar Jun 07 '24 05:06 rehanpfmr

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.

dmathieu avatar Jun 07 '24 07:06 dmathieu

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

rehanpfmr avatar Jun 07 '24 16:06 rehanpfmr

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?

dmathieu avatar Jun 07 '24 16:06 dmathieu

@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 avatar Jun 10 '24 10:06 rehanpfmr

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

rehanpfmr avatar Jun 21 '24 05:06 rehanpfmr

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

dmathieu avatar Jun 24 '24 07:06 dmathieu

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

@@          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 Coverage Δ
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go 82.1% <100.0%> (+1.0%) :arrow_up:
...ntation/github.com/gin-gonic/gin/otelgin/option.go 100.0% <100.0%> (ø)

codecov[bot] avatar Jun 24 '24 07:06 codecov[bot]

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

rehanpfmr avatar Jul 19 '24 07:07 rehanpfmr

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

rehanpfmr avatar Jul 25 '24 10:07 rehanpfmr

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

rehanpfmr avatar Jul 29 '24 06:07 rehanpfmr

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

rehanpfmr avatar Jul 31 '24 15:07 rehanpfmr

No. We need a second review.

dmathieu avatar Jul 31 '24 15:07 dmathieu

@hanyuancheung, Please help approve this PR. Thanks

rehanpfmr avatar Aug 02 '24 06:08 rehanpfmr

cc @open-telemetry/go-approvers

dmathieu avatar Aug 02 '24 07:08 dmathieu

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

Thanks

rehanpfmr avatar Aug 06 '24 15:08 rehanpfmr

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

This PR's change LGTM!

hanyuancheung avatar Aug 19 '24 01:08 hanyuancheung

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

rehanpfmr avatar Aug 19 '24 07:08 rehanpfmr

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

rehanpfmr avatar Aug 19 '24 07:08 rehanpfmr