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

Add call to AllowMissingFileDescriptors in lib/netext/grpcext/reflect.go #3871

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

Lordnibbler
Copy link
Contributor

@Lordnibbler Lordnibbler commented Jul 23, 2024

What?

Add call to AllowMissingFileDescriptors in lib/netext/grpcext/reflect.go.

Why?

When this option is enabled, the reflection service will tolerate the absence of certain file descriptors. This means that if a file descriptor is missing, the reflection service will still attempt to provide information about available services and methods, albeit potentially incomplete.

For context as to why we want this fix, we are experiencing similar issues in k6 using grpc server reflection to the issues reported by grpcurl in this issue: fullstorydev/grpcurl#453. We noted after grpcurl 1.9.0 was released with the call to AllowMissingFileDescriptors in place the following type of errors were resolved:

Failed to list methods for service "xxx.yyy": Symbol not found: xxx.yyy
caused by: File not found: openapi/v3/annotations.proto

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@Lordnibbler Lordnibbler requested a review from a team as a code owner July 23, 2024 18:24
@Lordnibbler Lordnibbler requested review from codebien and joanlopez and removed request for a team July 23, 2024 18:24
@Lordnibbler
Copy link
Contributor Author

@codebien @joanlopez any chance you can take a look at this small change? Thank you!

@Lordnibbler
Copy link
Contributor Author

@olegbespalov @mstoykov any chance someone can take a look at this small change? thank you so much! :)

@olegbespalov olegbespalov added this to the v0.54.0 milestone Jul 31, 2024
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Thanks for your contribution!

@Lordnibbler
Copy link
Contributor Author

Thanks @olegbespalov - can you help me to get a second review? I also saw what looked like a transient failure in the windows-based codecov github action, but I was not able to re-trigger it. I rebased the branch and pushed, but it did not trigger again.

@olegbespalov
Copy link
Contributor

@Lordnibbler, it might be some flaky test 😢

So far, I believe all is good 👍 Let's wait for the second reviewer. Luckily, it will be merged after the k6 v0.53 release, which is already formed.

@Lordnibbler
Copy link
Contributor Author

@olegbespalov thank you! is there anyone else to ping for second review, or are the folks I tagged already sufficient?

@olegbespalov
Copy link
Contributor

@Lordnibbler, there is no need for pinging; we're aware of it, but as I said, release v0.53 is coming, so that's why the team is busy with other things. No worries, we return to you as soon as possible 😉

@Lordnibbler
Copy link
Contributor Author

Thank you! :)

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

As @olegbespalov said, this is not expected to be included in v0.53. So, if you need it, you have to use the master branch after we merge this pull request.

@joanlopez joanlopez merged commit 441f4d6 into grafana:master Aug 21, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants