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

Refactor log #3446

Merged
merged 7 commits into from
Jun 29, 2024
Merged

Refactor log #3446

merged 7 commits into from
Jun 29, 2024

Conversation

yuhan6665
Copy link
Member

This is just an example of the code. I'm trying to:

  • remove all errors.generated.go file
  • define new log methods like LogInfo(ctx context.Context, msg ...interface{})
  • also show caller method name

new log:
[1350929471] proxy/freedom.(*Handler).Process: connection opened to tcp:127.0.0.1:46565, local endpoint 127.0.0.1:41428, remote endpoint 127.0.0.1:46565
old log:
[1350929471] proxy/freedom: connection opened to tcp:127.0.0.1:46565, local endpoint 127.0.0.1:41428, remote endpoint 127.0.0.1:46565

@yuhan6665
Copy link
Member Author

@RPRX

@Fangliding
Copy link
Member

~~how about #2521 ~~

pc, _, _, _ := runtime.Caller(2)
details := runtime.FuncForPC(pc).Name()
if len(details) >= trim {
details = details[trim:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about trimming the string until the first . too, so that (*Handler).Process disappears? then it is a pure refactor.

I am in favor of removing errors.generated.go, the file seemed annoying to copy for me.

Is there a runtime penalty for checking the stack like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could trim more, but I think method name could be helpful as well. Many logger specifically does that.
For runtime cost, old way use reflect too. Cost should be similar. In general I would prefer save people time vs cpu time

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general I would prefer save people time

get rid of config.pb.go please thank you

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you taking about whole use of Protobuf?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah. I suppose it's for the users API? biggest trouble in writing the first version of the transport was to install the right version of protobuf compiler

Copy link
Member Author

Choose a reason for hiding this comment

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

that's learning time, not wasted
A lot of people hate Protobuf, it's controversy around the original author of v2ray.
My take is it is better for backend API dev with built-in types and compatibility and encode efficiency. I think it is an interesting choice here, has both good and bad.
BTW my next priority going to improve on xray APIs..

@RPRX
Copy link
Member

RPRX commented Jun 18, 2024

Planned for 1.9

@yuhan6665 yuhan6665 marked this pull request as ready for review June 22, 2024 16:19
@yuhan6665
Copy link
Member Author

Given the likelihood of merge conflict with other work, I will merge to main later if no further comments..

@RPRX
Copy link
Member

RPRX commented Jun 25, 2024

Given the likelihood of merge conflict with other work, I will merge to main later if no further comments..

感觉在小版本改日志格式不太好,虽然我们的小版本已经......

@yuhan6665
Copy link
Member Author

Given the likelihood of merge conflict with other work, I will merge to main later if no further comments..

感觉在小版本改日志格式不太好,虽然我们的小版本已经......

那(如之前讨论)我可以加个字符串截取 这样就是纯 refactor?

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.

None yet

4 participants