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

feat(trace): add trace example for custom trace client and provider #3847

Merged
merged 10 commits into from
Oct 14, 2024

Conversation

houseme
Copy link
Member

@houseme houseme commented Oct 8, 2024

feat(trace): add trace for Custom trace client and provider

#3171 #3201 #3834

@houseme houseme requested review from gqcn and hailaz October 8, 2024 11:07
@gqcn
Copy link
Member

gqcn commented Oct 8, 2024

@houseme codes conflict

@gqcn
Copy link
Member

gqcn commented Oct 9, 2024

@houseme 我总感觉没有封装初始化TraceProvider的需要,因为封装后灵活性肯定缺失了。可以保留之前的otlpgrpc/otlphttp组件,提供的是默认的TraceProvider初始化实现,如果用户想要更灵活的Trace设置,就自己初始化Provider,咱们在提供的组件源码中注释说明一下就好了。这样对于咱们未来维护成本也低。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@houseme I always feel that there is no need to encapsulate and initialize TraceProvider, because the flexibility is definitely lost after encapsulation. The previous otlpgrpc/otlphttp components can be retained, and the default TraceProvider initialization implementation is provided. If the user wants a more flexible Trace setting, he can initialize the Provider himself. We will comment in the provided component source code. It'll be ready in one go. This will also reduce our future maintenance costs.

@gqcn gqcn added discuss We need discuss to make decision. enhancement labels Oct 9, 2024
@houseme
Copy link
Member Author

houseme commented Oct 9, 2024

@houseme 我总感觉没有封装初始化TraceProvider的需要,因为封装后灵活性肯定缺失了。可以保留之前的otlpgrpc/otlphttp组件,提供的是默认的TraceProvider初始化实现,如果用户想要更灵活的Trace设置,就自己初始化Provider,咱们在提供的组件源码中注释说明一下就好了。这样对于咱们未来维护成本也低。

或许把这一块的代码移到example/trace/provider 目录,提供一个示例,需要自定义的按照示例写。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@houseme I always feel that there is no need to encapsulate and initialize TraceProvider, because the flexibility is definitely lost after encapsulation. The previous otlpgrpc/otlphttp components can be retained, and the default TraceProvider initialization implementation is provided. If the user wants a more flexible Trace setting, he can initialize the Provider himself. We will comment in the provided component source code. It'll be ready in one go. This will also reduce our future maintenance costs.

Maybe move this piece of code to the example/trace/provider directory and provide an example. If you need to customize it, follow the example.

@gqcn
Copy link
Member

gqcn commented Oct 9, 2024

@houseme 我总感觉没有封装初始化TraceProvider的需要,因为封装后灵活性肯定缺失了。可以保留之前的otlpgrpc/otlphttp组件,提供的是默认的TraceProvider初始化实现,如果用户想要更灵活的Trace设置,就自己初始化Provider,咱们在提供的组件源码中注释说明一下就好了。这样对于咱们未来维护成本也低。

或许把这一块的代码移到example/trace/provider 目录,提供一个示例,需要自定义的按照示例写。

嗯,我觉得这样好。另外咱们现有组件完善下注释,说明下如果自定义的初始化参考哪里。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@houseme I always feel that there is no need to encapsulate and initialize TraceProvider, because the flexibility is definitely lost after encapsulation. The previous otlpgrpc/otlphttp components can be retained, and the default TraceProvider initialization implementation is provided. If the user wants a more flexible Trace setting, he can initialize the Provider himself. We will comment in the provided component source code. It'll be ready in one go. This will also reduce our future maintenance costs.

Maybe move this piece of code to the example/trace/provider directory and provide an example. If you need to customize it, follow the example.

Well, I think that's good. In addition, we will add comments to our existing components to explain where to refer to for customized initialization.

@houseme
Copy link
Member Author

houseme commented Oct 10, 2024

@houseme 我总感觉没有封装初始化TraceProvider的需要,因为封装后灵活性肯定缺失了。可以保留之前的otlpgrpc/otlphttp组件,提供的是默认的TraceProvider初始化实现,如果用户想要更灵活的Trace设置,就自己初始化Provider,咱们在提供的组件源码中注释说明一下就好了。这样对于咱们未来维护成本也低。

或许把这一块的代码移到example/trace/provider 目录,提供一个示例,需要自定义的按照示例写。

嗯,我觉得这样好。另外咱们现有组件完善下注释,说明下如果自定义的初始化参考哪里。

改完了,看看需要改进一下注释不

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@houseme I always feel that there is no need to encapsulate and initialize TraceProvider, because the flexibility is definitely lost after encapsulation. You can retain the previous otlpgrpc/otlphttp components, and provide the default TraceProvider initialization implementation. If users want more flexible Trace settings, they can initialize the Provider themselves. We will comment in the provided component source code. It'll be ready in one go. This will also reduce our future maintenance costs.

Maybe move this piece of code to the example/trace/provider directory and provide an example. If you need to customize it, follow the example.

Well, I think that's good. In addition, our existing components will be improved with comments to explain where to refer to for customized initialization.

After the modification, let’s see if the comments need to be improved.

@houseme houseme requested a review from gqcn October 10, 2024 07:03
@gqcn
Copy link
Member

gqcn commented Oct 10, 2024

#3847 (comment)

个人感觉这样挺好的,可以在otlpgrpcotlphttp包的注释中完善一下,说一下如果想要更灵活第配置Tracing参数,那么参数example的哪个文件,放个链接过去即可。

@houseme
Copy link
Member Author

houseme commented Oct 11, 2024

#3847 (comment)

个人感觉这样挺好的,可以在otlpgrpcotlphttp包的注释中完善一下,说一下如果想要更灵活第配置Tracing参数,那么参数example的哪个文件,放个链接过去即可。

改了,也加上注释说明

@gqcn gqcn merged commit 4b5f637 into master Oct 14, 2024
40 checks passed
@gqcn gqcn deleted the feature/trace-ratio branch October 14, 2024 13:30
@gqcn gqcn changed the title feat(trace): add trace for Custom trace client and provider feat(trace): add trace example for custom trace client and provider Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss We need discuss to make decision. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants