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

otelgrpc: Add Filter for stats handler #5196

Merged
merged 23 commits into from May 9, 2024

Conversation

ymtdzzz
Copy link
Contributor

@ymtdzzz ymtdzzz commented Mar 3, 2024

Description

Add filter for stats handler and move interceptor filter to go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/filters/interceptor considering that it'll be deprecated.

Related Issue

Fixes #4575

Test

Unit tests added.
And I've tested on: https://github.com/grpc/grpc-go/tree/master/examples/route_guide

No filters

image

Method name filter

	opts = append(opts, grpc.StatsHandler(otelgrpc.NewServerHandler(
		otelgrpc.WithFilter(filters.MethodName("ListFeatures")),
	)))

image

@ymtdzzz ymtdzzz changed the title [WIP] otelgrpc: Add Filter for stats handler otelgrpc: Add Filter for stats handler Mar 3, 2024
@ymtdzzz ymtdzzz marked this pull request as ready for review March 3, 2024 08:13
@ymtdzzz ymtdzzz requested review from dashpole, hanyuancheung and a team as code owners March 3, 2024 08:13
CHANGELOG.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor

dashpole commented Mar 4, 2024

lgtm overall.

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 88.42105% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 62.7%. Comparing base (d6e2fc4) to head (95bca4e).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5196     +/-   ##
=======================================
+ Coverage   62.5%   62.7%   +0.1%     
=======================================
  Files        191     192      +1     
  Lines      11727   11786     +59     
=======================================
+ Hits        7340    7399     +59     
  Misses      4170    4170             
  Partials     217     217             
Files Coverage Δ
...google.golang.org/grpc/otelgrpc/filters/filters.go 100.0% <100.0%> (+6.0%) ⬆️
...n/google.golang.org/grpc/otelgrpc/stats_handler.go 100.0% <100.0%> (ø)
...entation/google.golang.org/grpc/otelgrpc/config.go 89.0% <50.0%> (+0.6%) ⬆️
...g.org/grpc/otelgrpc/filters/interceptor/filters.go 93.9% <93.9%> (ø)
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 89.4% <0.0%> (ø)

@pellared pellared requested a review from dashpole March 6, 2024 11:19
@jmjesperson
Copy link

Any update if this will be reviewed in order to be merged?

@ymtdzzz
Copy link
Contributor Author

ymtdzzz commented May 6, 2024

@dashpole @hanyuancheung
Please let me know if there is any additional work required for this PR to be merged :)

CHANGELOG.md Outdated Show resolved Hide resolved
pellared
pellared previously approved these changes May 6, 2024
@pellared
Copy link
Member

pellared commented May 6, 2024

Overall LGTM, just little comments that I think it would be nice to address before merging.

CHANGELOG.md Show resolved Hide resolved
@pellared pellared dismissed their stale review May 8, 2024 09:50

LGTM, but there are some comments that should be addressed.

@pellared
Copy link
Member

pellared commented May 8, 2024

@dashpole, can you please make another review and possibly merge if you find everything good?

@pellared pellared requested a review from dashpole May 8, 2024 12:55
@MrAlias MrAlias merged commit 0483033 into open-telemetry:main May 9, 2024
23 checks passed
@ymtdzzz ymtdzzz deleted the grpc-stats-filter branch May 9, 2024 15:12
@pgporada
Copy link

Hey thanks!

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.

otelgrpc: stats handler doesn't seem to have an option for filtering
7 participants