grpc/reflection: Multiple services in the same proto #2265

codebien · 2021-11-25T18:56:17Z

It fixes the problem with multiple services defined in the same proto file. It was originally reported in the forum: https://community.k6.io/t/v0-35-0-grpc-server-reflection-error/2383/8

For summary:
In the case, a single proto file defines multiple services the grpc.connect function returns the can’t convert method info: proto: file appears multiple times: “tenant/tenant_service.proto error.

A commit with the gRPC sample edited it's included so it can be tested easily by reviewers. It includes the route_guide.proto file updated that splits part of the methods in a second service. It's expected to be removed before merging.

TODO

Report back in the forum when this PR is going to be merged/has been merged

yorugac · 2021-11-30T09:55:33Z

Hi @codebien could you please add a brief description of the bug here? It seems like the key info was in forum's DM and though some guesses can be made looking at the code, I think it'd be more precise coming from you 🙂

codebien · 2021-11-30T10:05:52Z

Hi @yorugac, I have updated the description, let me know if it's better now. Thanks!

inancgumus

LGTM functionally.

js/modules/k6/grpc/client.go

yorugac

Yes, that's clear, thank you! I haven't tried to run any more complex options but it seems to be fine 👍

There are some minor typos, in case you'll be making any further changes to the PR 🙂

samples/grpc_server/main.go

js/modules/k6/grpc/client.go

mstoykov

LGTM, I can't see anything wrong with this and looking at the documentation I don't see anyway in which we can skip doing it and instead use something from the grpc library so 🤷 I guess we need to do it "manually"

codebien · 2021-12-01T15:06:41Z

I removed the commit with sample changes as pre-announced in the description

mstoykov

Can you still expand the samples in a separate PR?

codebien · 2021-12-01T15:27:34Z

@mstoykov if we want that then I prefer to do it directly here.

codebien · 2021-12-01T17:28:04Z

samples/grpc_server/route_guide.proto

-// Copyright 2015 gRPC authors.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-


Should we keep it now that we changed it?

Yeah, I think we must keep it, but maybe also either do something like this:

Copyright 2015 gRPC authors, with some modification by the k6 team (2021).

or just leave it as it is 🤷‍♂️

@na-- sorry, I pushed before I was able to read your comment, I will add your suggestion in the second PR that we expect to create.

codebien · 2021-12-01T17:28:28Z

Added the sample as requested

codebien · 2021-12-13T16:22:15Z

@oleiade Added a dedicated issue #2295 to address that, this version covers testing manually with the updated sample.

mstoykov · 2021-12-14T08:20:05Z

samples/grpc_server/route_guide.pb.go

-// Copyright 2015 gRPC authors.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-


Not sure if we can delete this and not sure how it was generated in the first place 🤔

I restored it but the latest generator doesn't add it so I thought they decided to not add it anymore

It asserts that the grpc reflection feature works also when multiple services are defined in the same proto file.

codebien · 2021-12-14T09:15:34Z

Regenerated the file instead to re-add it manually

mstoykov

LGTM.

There is still no copyright header on the second file, but I do think that the generated code is probably less copyrightable 🤷

It's an extension of #2265

It's an extension of grafana/k6#2265

codebien self-assigned this Nov 25, 2021

codebien marked this pull request as ready for review November 26, 2021 09:50

github-actions bot requested review from na-- and yorugac November 26, 2021 09:50

codebien requested a review from inancgumus November 26, 2021 09:50

inancgumus previously approved these changes Nov 30, 2021

View reviewed changes

js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved

yorugac previously approved these changes Nov 30, 2021

View reviewed changes

samples/grpc_server/main.go Outdated Show resolved Hide resolved

js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved

mstoykov dismissed inancgumus’s stale review via 4bd8db0 November 30, 2021 13:43

codebien dismissed yorugac’s stale review via 50459db November 30, 2021 18:27

codebien force-pushed the grpc-2383-forum branch from 4bd8db0 to 50459db Compare November 30, 2021 18:27

codebien requested review from yorugac and inancgumus November 30, 2021 18:29

yorugac previously approved these changes Dec 1, 2021

View reviewed changes

mstoykov previously approved these changes Dec 1, 2021

View reviewed changes

inancgumus previously approved these changes Dec 1, 2021

View reviewed changes

codebien dismissed stale reviews from inancgumus, mstoykov, and yorugac via a9bc5b7 December 1, 2021 15:03

codebien force-pushed the grpc-2383-forum branch from 50459db to a9bc5b7 Compare December 1, 2021 15:03

codebien added this to the v0.36.0 milestone Dec 1, 2021

inancgumus previously approved these changes Dec 1, 2021

View reviewed changes

mstoykov previously approved these changes Dec 1, 2021

View reviewed changes

codebien dismissed stale reviews from mstoykov and inancgumus via dfd224a December 1, 2021 17:26

codebien commented Dec 1, 2021

View reviewed changes

codebien dismissed stale reviews from oleiade and yorugac via 391ddef December 13, 2021 15:58

codebien force-pushed the grpc-2383-forum branch from dfd224a to 391ddef Compare December 13, 2021 15:58

codebien requested review from na--, inancgumus, oleiade and yorugac December 13, 2021 16:15

codebien mentioned this pull request Dec 13, 2021

grpc: Unit test the multi services fix #2295

Closed

mstoykov reviewed Dec 14, 2021

View reviewed changes

codebien force-pushed the grpc-2383-forum branch from 391ddef to 7097e04 Compare December 14, 2021 08:27

oleiade previously approved these changes Dec 14, 2021

View reviewed changes

samples/grpc: multiple services in the same proto

f7dea61

It asserts that the grpc reflection feature works also when multiple services are defined in the same proto file.

codebien dismissed oleiade’s stale review via f7dea61 December 14, 2021 09:15

codebien force-pushed the grpc-2383-forum branch from 7097e04 to f7dea61 Compare December 14, 2021 09:15

mstoykov approved these changes Dec 14, 2021

View reviewed changes

oleiade self-requested a review December 14, 2021 10:18

oleiade approved these changes Dec 14, 2021

View reviewed changes

yorugac approved these changes Dec 14, 2021

View reviewed changes

olegbespalov approved these changes Dec 14, 2021

View reviewed changes

codebien merged commit 703fd57 into master Dec 14, 2021

codebien deleted the grpc-2383-forum branch December 14, 2021 10:22

codebien added a commit that referenced this pull request Dec 16, 2021

js/grpc: Tests for multi services in the same proto file

ff36831

It's an extension of #2265

codebien added a commit that referenced this pull request Dec 16, 2021

js/grpc: Tests for multi services in the same proto file

5d6bca4

It's an extension of #2265

codebien added a commit that referenced this pull request Dec 24, 2021

js/grpc: Tests for multi services in the same proto file

095d429

It's an extension of #2265

codebien added a commit that referenced this pull request Jan 11, 2022

js/grpc: Tests for multi services in the same proto file

ede3503

It's an extension of #2265

codebien added a commit that referenced this pull request Jan 11, 2022

js/grpc: Tests for multi services in the same proto file

19a5ac4

It's an extension of #2265

codebien added a commit that referenced this pull request Jan 11, 2022

js/grpc: Tests for multi services in the same proto file

87504c9

It's an extension of #2265

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

js/grpc: Tests for multi services in the same proto file

7a96e0a

It's an extension of grafana/k6#2265

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc/reflection: Multiple services in the same proto #2265

grpc/reflection: Multiple services in the same proto #2265

codebien commented Nov 25, 2021 •

edited

Loading

yorugac commented Nov 30, 2021

codebien commented Nov 30, 2021 •

edited

Loading

inancgumus left a comment

yorugac left a comment

mstoykov left a comment

codebien commented Dec 1, 2021

mstoykov left a comment

codebien commented Dec 1, 2021

codebien Dec 1, 2021

na-- Dec 13, 2021

codebien Dec 13, 2021 •

edited

Loading

codebien commented Dec 1, 2021

codebien commented Dec 13, 2021 •

edited

Loading

mstoykov Dec 14, 2021

codebien Dec 14, 2021

codebien commented Dec 14, 2021

mstoykov left a comment

grpc/reflection: Multiple services in the same proto #2265

grpc/reflection: Multiple services in the same proto #2265

Conversation

codebien commented Nov 25, 2021 • edited Loading

yorugac commented Nov 30, 2021

codebien commented Nov 30, 2021 • edited Loading

inancgumus left a comment

Choose a reason for hiding this comment

yorugac left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Dec 1, 2021

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Dec 1, 2021

codebien Dec 1, 2021

Choose a reason for hiding this comment

na-- Dec 13, 2021

Choose a reason for hiding this comment

codebien Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

codebien commented Dec 1, 2021

codebien commented Dec 13, 2021 • edited Loading

mstoykov Dec 14, 2021

Choose a reason for hiding this comment

codebien Dec 14, 2021

Choose a reason for hiding this comment

codebien commented Dec 14, 2021

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Nov 25, 2021 •

edited

Loading

codebien commented Nov 30, 2021 •

edited

Loading

codebien Dec 13, 2021 •

edited

Loading

codebien commented Dec 13, 2021 •

edited

Loading