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

Client grpc mw #12

Merged
merged 13 commits into from
Nov 14, 2023
Merged

Client grpc mw #12

merged 13 commits into from
Nov 14, 2023

Conversation

glebnaz
Copy link
Owner

@glebnaz glebnaz commented Nov 13, 2023

Add:

  • client mw for: trace,log,metrics
  • fix some server metrics var (check the initialisation)

@glebnaz glebnaz self-assigned this Nov 13, 2023
metrics/grpc_mw.go Show resolved Hide resolved
metrics/grpc_mw_client.go Outdated Show resolved Hide resolved
metrics/grpc_mw_client.go Show resolved Hide resolved
metrics/grpc_mw_client.go Outdated Show resolved Hide resolved
metrics/grpc_mw.go Outdated Show resolved Hide resolved
metrics/grpc_mw.go Outdated Show resolved Hide resolved
Comment on lines +10 to +13
type FromKeyType string

// FromKey is the key hold from service name
var FromKey FromKeyType = "x-api-from"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Они внешние из-за какой то идеи? Или нет?

Copy link
Owner Author

Choose a reason for hiding this comment

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

что бы переиспользовать outside

Comment on lines 26 to 43
reqID := GetSimpleReqIDFromContext(ctx)
if reqID != "" {
md, ok := metadata.FromOutgoingContext(ctx)
if ok {
md = metadata.Join(md, metadata.New(map[string]string{string(simpleReqID): reqID}))
ctx = metadata.NewOutgoingContext(ctx, md)
} else {
ctx = metadata.NewOutgoingContext(ctx, metadata.New(map[string]string{string(simpleReqID): reqID}))
}
} else {
ctx, reqID = MustGetSimpleReqIDFromContext(ctx)
md, ok := metadata.FromOutgoingContext(ctx)
if ok {
md = metadata.Join(md, metadata.New(map[string]string{string(simpleReqID): reqID}))
ctx = metadata.NewOutgoingContext(ctx, md)
}
ctx = metadata.NewOutgoingContext(ctx, metadata.New(map[string]string{string(simpleReqID): reqID}))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Меня напрягает эта конструкция. Не очень понятно почему нельзя сразу вызвать MustGetSimpleReqIDFromContext(ctx). И не очень понятно почему при наличии наличии reqID у нас стоит else конструкция, а при генерации её уже нет.

Comment on lines +67 to +70
returnFunc := func(ctx context.Context, reqID string) (string, context.Context) {
ctx = context.WithValue(ctx, simpleReqID, reqID)
return reqID, ctx
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не слишком ли мудрено написано? С функцией GetSimpleReqIDFromMetaData достаточно много общего. Мне кажется можно просто вызвать в этой функции функцию GetSimpleReqIDFromMetaData(), и немного упростить читаемость. Мое мнение что этот паттерн тут прям явно лишний

Copy link
Owner Author

Choose a reason for hiding this comment

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

так это совсем разные функции

Copy link
Collaborator

@mike-barshev mike-barshev Nov 14, 2023

Choose a reason for hiding this comment

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

я вижу что функции одинаковые. Просто одна из низ возвращает пустой reqID, в случае если он пустой, а вторая генерирует его. Я про то, что это можно сделать как то так (я мог ошибиться, перепроверь)

func MustGetSimpleReqIDFromMetaData(ctx context.Context) (string, context.Context) {
	reqID, ctx := GetSimpleReqIDFromMetaData(ctx)
	if reqID == "" {
		reqID = GenerateSimpleReqID()
		ctx = context.WithValue(ctx, simpleReqID, reqID)
		return reqID, ctx
	}
	return reqID, ctx
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

ААА это, ну must это типа сгенерить принудительно)

это паттерн такой

@glebnaz glebnaz merged commit 5b660e7 into develop Nov 14, 2023
2 checks passed
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.

2 participants