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: allow retrieving data from metadata or adding data to metadata #45

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rogerogers
Copy link
Collaborator

允许从 metadata 获取信息、设置信息到 metadata

closed: cloudwego/kitex#1323

Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rogerogers
To complete the pull request process, please assign coderpoet after the PR has been reviewed.
You can assign the PR to them by writing /assign @coderpoet in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rogerogers rogerogers closed this Apr 20, 2024
@rogerogers rogerogers deleted the feat/metadata-extractor branch April 20, 2024 07:24
@rogerogers rogerogers restored the feat/metadata-extractor branch April 20, 2024 07:25
@rogerogers rogerogers reopened this Apr 20, 2024
Signed-off-by: rogerogers <[email protected]>
Signed-off-by: rogerogers <[email protected]>
"go.opentelemetry.io/otel/propagation"
)

type metadataSupplier struct {
Copy link
Member

Choose a reason for hiding this comment

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

这个看起来和目前的 propagator 区别不大? https://github.com/kitex-contrib/obs-opentelemetry/blob/main/tracing/propagator.go

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

嗯,主要是考虑是否需要区分 grpc metadata 和普通的 metadata

@@ -43,6 +44,13 @@ func ClientMiddleware(cfg *config) endpoint.Middleware {

Inject(ctx, cfg, md)

if cfg.enableMetadata {
grpcMd, ok := metadata.FromOutgoingContext(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

是不是可以考虑合并下 md,而非重复实现一个 propogator?

Copy link
Member

Choose a reason for hiding this comment

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

可能得考虑一个场景,比如:a -> b -> c

a -> b 走的 kitex thrift 协议, b -> c 走的 kitex grpc 协议

那 a -> b 的 metadata 是否需要通过 grpc metadata 的方式(metadata.AppendToOutgoingContext)透传给 c

ctx = injectMetadata(ctx, cfg, grpcMd)
}
}

for k, v := range md {
ctx = metainfo.WithValue(ctx, k, v)
Copy link
Member

Choose a reason for hiding this comment

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

grpc 的 metadata 传输方式也是这样么?还是说得 metadata.AppendToOutgoingContext

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

metadata.NewOutgoingContextmetadata.AppendToOutgoingContext 效果差不多

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.

Feature Proposal: tracing.ServerMiddleware 同时支持 metainfo 和 metadata 协议读取
2 participants