Introducing the reflection metadata param #51
Conversation
olegbespalov
requested review from
mstoykov and
codebien
and removed request for
a team
olegbespalov
changed the title
| @@ -56,6 +62,42 @@ func TestParamsInvalidInput(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestCallParamsMetadata(t *testing.T) { | |||
There was a problem hiding this comment.
| func TestCallParamsMetadata(t *testing.T) { | |
| func TestNewCallParamsMetadata(t *testing.T) { |
There was a problem hiding this comment.
Could you please elaborate on why you believe that this is an improvement?
There was a problem hiding this comment.
It matches the SUT method
There was a problem hiding this comment.
Actually, it's not. The SUT for that test case callParams' metadata
| // The gRPC spec defines that Binary-valued keys end in -bin | ||
| // https://grpc.io/docs/what-is-grpc/core-concepts/#metadata |
There was a problem hiding this comment.
I think we should log it directly in the error message (excluding the link), so we can drop the comment here and have a better error message at the same time.
There was a problem hiding this comment.
I don't think the suggestion could improve the error message and generally fit the context of the log error message.
I prefer to stick with your original suggestion, where the comment serves just as for contributors/maintainers and explains the difference in treatment keys with postfixes, basically explaining why this if exists.
But for sure, we should explain this difference in the documentation, which we will probably do in an upcoming docs PR.
There was a problem hiding this comment.
I don't think the suggestion could improve the error message and generally fit the context of the log error message.
It gives you concrete feedback/suggestions about how to fix the issue. If a user reads the docs then I don't expect they will hit the issue, if they are hitting the issue probably they aren't aware of it.
This suggestion is still aligned with the original proposal, but I would extend it to an eventual user hitting the issue. We may preserve the link as a comment if we need more info as developers.
Anyway, this is just a suggestion, feel free to discard it.
There was a problem hiding this comment.
It gives you concrete feedback/suggestions about how to fix the issue.
But that's my point that it's not. Moreover, it could easily confuse the person if we give a link to the GRPC documentation where no k6's specific (like how to pass the binary data) is mentioned.
The error that came from this method is always about the type, but reading the https://grpc.io/docs/what-is-grpc/core-concepts/#metadata doesn't always answer, for instance, the case below will trigger the error:
metadata: {foo: 1}
If we're talking about the error that is coming from the -bin key, the page also doesn't help since it doesn't specify how to define binary data inside the k6.
And as a result, it might be useful only for the people who read the code and trying to get why this is special treatment of such keys exists.
So yeah, sorry, but I'm going to keep it as it is right now.
| foundReflectionCall = true | ||
|
|
||
| // check that the metadata is present | ||
| assert.Contains(t, entry.Message, "x-test: custom-header-for-reflection") |
There was a problem hiding this comment.
Can we use LogContains or FilterEntries here? https://github.com/grafana/k6/blob/981b770946f5df5d772554e5906f888ef1ca8a94/lib/testutils/logrus_hook.go#L82
There was a problem hiding this comment.
Not really. We do check that specific RPC call contains the headers. The method that you're referring to works for the less complex cases.
This minor refactoring is a pre-requisition of the following extraction of the logic of parsing a gRPC metadata.
olegbespalov
force-pushed
the
feat/reflect-metadata
branch
from
200e445
to
c9ea516
Compare
olegbespalov
force-pushed
the
feat/reflect-metadata
branch
from
c9ea516
to
23521f0
Compare
olegbespalov
force-pushed
the
feat/reflect-metadata
branch
from
23521f0
to
1b3ca79
Compare
Extracting logic + few more tests
olegbespalov
force-pushed
the
feat/reflect-metadata
branch
from
1b3ca79
to
350679e
Compare
What?
Introduce a new connection parameter that helps configure metadata for reflection calls.
This PR also contains a minor refactoring that helps re-use some parts of the logic, so it's better to review by commits.
Why?
Even the reflection calls sometimes require metadata.
Closes: #33