Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Set http default method to "/" #2168

Merged
merged 1 commit into from
Apr 26, 2023
Merged

Conversation

chenBright
Copy link
Contributor

What problem does this PR solve?

Issue Number:

Problem Summary:

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects(性能影响):

  • Breaking backward compatibility(向后兼容性):


Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 17, 2023

这个是否会影响代码的向前兼容性?

@chenBright
Copy link
Contributor Author

应该不影响,目前method name只在创建client span的时候使用了而已。

brpc/src/brpc/channel.cpp

Lines 470 to 479 in fc885ad

if (_get_method_name) {
method_name = &_get_method_name(method, cntl);
} else if (method) {
method_name = &method->full_name();
} else {
const static std::string NULL_METHOD_STR = "null-method";
method_name = &NULL_METHOD_STR;
}
Span* span = Span::CreateClientSpan(
*method_name, start_send_real_us - start_send_us);

@chenBright
Copy link
Contributor Author

如果担心有影响的话,也可以在CallMethod中,_get_method_name后method_name为空再设置为"/"。

@wwbmmm
Copy link
Contributor

wwbmmm commented Apr 17, 2023

LGTM

@wwbmmm wwbmmm merged commit 8f4dd63 into apache:master Apr 26, 2023
@chenBright chenBright deleted the http_default_method branch April 26, 2023 08:17
yanglimingcn pushed a commit to yanglimingcn/brpc that referenced this pull request Jun 25, 2023
yanglimingcn pushed a commit to yanglimingcn/brpc that referenced this pull request Oct 31, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants