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: Add *gin.Context Filter parameter #5743

Merged
merged 8 commits into from
Aug 19, 2024

Conversation

rehanpfmr
Copy link
Contributor

Issue: #3070
last PR status: #4444

@rehanpfmr rehanpfmr requested a review from a team June 7, 2024 05:18
@dmathieu
Copy link
Member

dmathieu commented Jun 7, 2024

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 dmathieu marked this pull request as draft June 7, 2024 07:08
@rehanpfmr rehanpfmr marked this pull request as ready for review June 7, 2024 13:21
@rehanpfmr
Copy link
Contributor Author

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

@dmathieu
Copy link
Member

dmathieu commented Jun 7, 2024

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
Copy link
Contributor Author

@dmathieu
Please refer to #4444, the solution was recommended here.
I'm looking at the Test for "TestWithGinFilter" is currently failing.

@dmathieu dmathieu changed the title feat: Add *gin.Context Filter parameter https://github.com/open-telemetry/opentelemetry-go-contrib/issues/3070 #3070 feat: Add *gin.Context Filter parameter Jun 10, 2024
@rehanpfmr
Copy link
Contributor Author

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

@rehanpfmr rehanpfmr requested a review from dmathieu June 21, 2024 13:35
@dmathieu
Copy link
Member

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

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️
...ntation/github.com/gin-gonic/gin/otelgin/option.go 100.0% <100.0%> (ø)

rehanpfmr added a commit to fidelity-contributions/open-telemetry-opentelemetry-go-contrib that referenced this pull request Jul 19, 2024
commit f5116f4472a90ace1b336eecae28589d77311d92
Author: Rehan Pasha <rehan.pasha@fmr.com>
Date:   Tue Jun 25 08:04:38 2024 -0400

    Fixing lint error

    Signed-off-by: Rehan pasha <rehan.pasha@fmr.com>

commit 084e672
Author: Rehan Pasha <rehan.pasha@fmr.com>
Date:   Tue Jun 11 14:42:26 2024 +0530

    Squashed commit of the following:

    commit e6f9504a62f70f45c9e742a054ce05187f175176
    Author: Pasha, Rehan <Rehan.Pasha@fmr.com>
    Date:   Tue Jun 11 14:34:25 2024 +0530

        Update gintrace.go

        Signed-off-by: Pasha, Rehan <Rehan.Pasha@fmr.com>

    commit 8f532bdd4dee57093934a2ce3f717b3ad657d0c4
    Author: Pasha, Rehan <Rehan.Pasha@fmr.com>
    Date:   Tue Jun 11 14:31:49 2024 +0530

        Update option.go

        Signed-off-by: Pasha, Rehan <Rehan.Pasha@fmr.com>

    commit 52b9271c55f98f64bd10838209bde7fcced30dcc
    Merge: ec30e0b1 c7b10074
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:30:06 2024 +0530

        Merge branch 'rehanosp' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

    commit ec30e0b158ca3a00c6df0e5495f34f990a59c153
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:29:42 2024 +0530

        Fixing the test

    commit c7b10074e6e4b90b16b384fe661d351d8ffbe035
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:23:20 2024 +0530

        Fixing the test

    commit 969a646a0cf4f0d430fd5fa4255f65c52d5bfee5
    Merge: 737ff9e 8b12e62
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:06:24 2024 +0530

        Merge branch 'main' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

    commit 737ff9e
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Fri Jun 7 18:33:59 2024 +0530

        Squashed commit of the following:

        commit 93a2b553456d9bbb19b59da9d1e611ee096412a7
        Merge: 73dd86e7 85969a3
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Fri Jun 7 18:25:10 2024 +0530

            Merge branch 'main' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

        commit 73dd86e74e37ce595707fd8daa75df1af934706a
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Fri Jun 7 16:17:30 2024 +0530

            feat: Update WithGinFilter to use GinFilter type

            The `WithGinFilter` function in `option.go` has been updated to use the `GinFilter` type instead of the generic `Filter` type. This change ensures that only `GinFilter` instances are added to the list of filters used by the handler.

        commit c0330a053d35a2294ae2010ab3bc13eb8a6906b3
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Fri Jun 7 15:42:30 2024 +0530

            Fixing the filter and adding test open-telemetry#5743 (comment)

        commit 1facc34
        Merge: ce53f63 3488eb8
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:50:32 2024 +0530

            Merge branch 'rehanosp' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

        commit ce53f63
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:50:12 2024 +0530

            I’ve added my DCO signoff at the project’s request. There are no other changes.

            Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

        commit 3488eb8
        Merge: cce7c22 606c275
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:35:07 2024 +0530

            Merge branch 'rehanosp' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

        commit cce7c22
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:34:48 2024 +0530

            feat: Add *gin.Context Filter parameter

            Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

        commit 606c275
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Mon May 20 19:59:53 2024 +0530

            feat: Add `*gin.Context` Filter parameter in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`

            open-telemetry#3070

        Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

    Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>
@rehanpfmr
Copy link
Contributor Author

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

@rehanpfmr
Copy link
Contributor Author

rehanpfmr commented Jul 25, 2024

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

@rehanpfmr
Copy link
Contributor Author

rehanpfmr commented Jul 29, 2024

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

CHANGELOG.md Outdated Show resolved Hide resolved
rehanpfmr and others added 5 commits July 30, 2024 16:00
commit f5116f4472a90ace1b336eecae28589d77311d92
Author: Rehan Pasha <rehan.pasha@fmr.com>
Date:   Tue Jun 25 08:04:38 2024 -0400

    Fixing lint error

    Signed-off-by: Rehan pasha <rehan.pasha@fmr.com>

commit 084e672
Author: Rehan Pasha <rehan.pasha@fmr.com>
Date:   Tue Jun 11 14:42:26 2024 +0530

    Squashed commit of the following:

    commit e6f9504a62f70f45c9e742a054ce05187f175176
    Author: Pasha, Rehan <Rehan.Pasha@fmr.com>
    Date:   Tue Jun 11 14:34:25 2024 +0530

        Update gintrace.go

        Signed-off-by: Pasha, Rehan <Rehan.Pasha@fmr.com>

    commit 8f532bdd4dee57093934a2ce3f717b3ad657d0c4
    Author: Pasha, Rehan <Rehan.Pasha@fmr.com>
    Date:   Tue Jun 11 14:31:49 2024 +0530

        Update option.go

        Signed-off-by: Pasha, Rehan <Rehan.Pasha@fmr.com>

    commit 52b9271c55f98f64bd10838209bde7fcced30dcc
    Merge: ec30e0b1 c7b10074
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:30:06 2024 +0530

        Merge branch 'rehanosp' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

    commit ec30e0b158ca3a00c6df0e5495f34f990a59c153
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:29:42 2024 +0530

        Fixing the test

    commit c7b10074e6e4b90b16b384fe661d351d8ffbe035
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:23:20 2024 +0530

        Fixing the test

    commit 969a646a0cf4f0d430fd5fa4255f65c52d5bfee5
    Merge: 737ff9e 8b12e62
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Tue Jun 11 14:06:24 2024 +0530

        Merge branch 'main' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

    commit 737ff9e
    Author: Rehan Pasha <rehan.pasha@fmr.com>
    Date:   Fri Jun 7 18:33:59 2024 +0530

        Squashed commit of the following:

        commit 93a2b553456d9bbb19b59da9d1e611ee096412a7
        Merge: 73dd86e7 85969a3
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Fri Jun 7 18:25:10 2024 +0530

            Merge branch 'main' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

        commit 73dd86e74e37ce595707fd8daa75df1af934706a
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Fri Jun 7 16:17:30 2024 +0530

            feat: Update WithGinFilter to use GinFilter type

            The `WithGinFilter` function in `option.go` has been updated to use the `GinFilter` type instead of the generic `Filter` type. This change ensures that only `GinFilter` instances are added to the list of filters used by the handler.

        commit c0330a053d35a2294ae2010ab3bc13eb8a6906b3
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Fri Jun 7 15:42:30 2024 +0530

            Fixing the filter and adding test open-telemetry#5743 (comment)

        commit 1facc34
        Merge: ce53f63 3488eb8
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:50:32 2024 +0530

            Merge branch 'rehanosp' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

        commit ce53f63
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:50:12 2024 +0530

            I’ve added my DCO signoff at the project’s request. There are no other changes.

            Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

        commit 3488eb8
        Merge: cce7c22 606c275
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:35:07 2024 +0530

            Merge branch 'rehanosp' of https://github.com/fidelity-external-staging/open-telemetry-opentelemetry-go-contrib into rehanosp

        commit cce7c22
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Tue May 28 13:34:48 2024 +0530

            feat: Add *gin.Context Filter parameter

            Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

        commit 606c275
        Author: Rehan Pasha <rehan.pasha@fmr.com>
        Date:   Mon May 20 19:59:53 2024 +0530

            feat: Add `*gin.Context` Filter parameter in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`

            open-telemetry#3070

        Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

    Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>

Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>
Signed-off-by: Brian Warner <brian.warner2@fmr.com>
Signed-off-by: Rehan Pasha <rehan.pasha@fmr.com>
Signed-off-by: Brian Warner <brian.warner2@fmr.com>
Signed-off-by: rehanpfmr <111350825+rehanpfmr@users.noreply.github.com>
Co-authored-by: Damien Mathieu <42@dmathieu.com>
Signed-off-by: rehanpfmr <111350825+rehanpfmr@users.noreply.github.com>
Signed-off-by: rehanpfmr <111350825+rehanpfmr@users.noreply.github.com>
Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

Approving despite lack of review/opinion from @hanyuancheung on API change.

@rehanpfmr
Copy link
Contributor Author

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

@dmathieu
Copy link
Member

No. We need a second review.

@rehanpfmr
Copy link
Contributor Author

@hanyuancheung, Please help approve this PR.
Thanks

@dmathieu
Copy link
Member

dmathieu commented Aug 2, 2024

cc @open-telemetry/go-approvers

@rehanpfmr
Copy link
Contributor Author

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

Thanks

@hanyuancheung
Copy link
Member

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

This PR's change LGTM!

@rehanpfmr
Copy link
Contributor Author

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

@dmathieu dmathieu merged commit 35d271f into open-telemetry:main Aug 19, 2024
27 checks passed
@rehanpfmr
Copy link
Contributor Author

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

@MrAlias MrAlias added this to the v1.29.0 milestone Aug 22, 2024
MrAlias added a commit that referenced this pull request Aug 23, 2024
This release is the last to support [Go 1.21]. The next release will
require at least [Go 1.22].

### Added

- Add the `WithSpanAttributes` and `WithMetricAttributes` methods to set
custom attributes to the stats handler in
`go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`.
(#5133)
- The `go.opentelemetry.io/contrib/bridges/otelzap` module. This module
provides an OpenTelemetry logging bridge for `go.uber.org/zap`. (#5191)
- Support for the `OTEL_HTTP_CLIENT_COMPATIBILITY_MODE=http/dup`
environment variable in
`go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` to emit
attributes for both the v1.20.0 and v1.26.0 semantic conventions.
(#5401)
- The `go.opentelemetry.io/contrib/bridges/otelzerolog` module. This
module provides an OpenTelemetry logging bridge for
`github.com/rs/zerolog`. (#5405)
- Add `WithGinFilter` filter parameter in
`go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`
to allow filtering requests with `*gin.Context`. (#5743)
- Support for stdoutlog exporter in
`go.opentelemetry.io/contrib/config`. (#5850)
- Add macOS ARM64 platform to the compatibility testing suite. (#5868)
- Add new runtime metrics to
`go.opentelemetry.io/contrib/instrumentation/runtime`, which are still
disabled by default. (#5870)
- Add the `WithMetricsAttributesFn` option to allow setting dynamic,
per-request metric attributes in
`go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#5876)
- The `go.opentelemetry.io/contrib/config` package supports configuring
`with_resource_constant_labels` for the prometheus exporter. (#5890)
- Support [Go 1.23]. (#6017)

### Removed

- The deprecated `go.opentelemetry.io/contrib/processors/baggagecopy`
package is removed. (#5853)

### Fixed

- Race condition when reading the HTTP body and writing the response in
`go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`. (#5916)

[Go 1.23]: https://go.dev/doc/go1.23
[Go 1.22]: https://go.dev/doc/go1.22
[Go 1.21]: https://go.dev/doc/go1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants