-
Notifications
You must be signed in to change notification settings - Fork 29
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(frontend): search spans from extension tracers when object name is specified #369
base: main
Are you sure you want to change the base?
Conversation
df20cd3
to
0e47d68
Compare
0e47d68
to
756f654
Compare
pkg/frontend/reader/merge/merge.go
Outdated
@@ -237,14 +237,24 @@ func newObject[M any](key objKey, trace *tftree.SpanTree, metadata M) (*object[M | |||
if err != nil { | |||
return nil, fmt.Errorf("clone spans: %w", err) | |||
} | |||
|
|||
var m []M | |||
if !isNil(metadata) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isNil
will always be false unless M
is an interface. It is best not to rely on such unexpected behavior.
What problem are you trying to solve here? Is it better to change trace.Metadata
itself to a slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SOF3 if trace.Metadata is nil,slice长度为1,identifiers序列化之后变成了字符串null,读缓存的时候没有办法再去判断这个null是个什么东西。 trace.Metadata 改为slice是可以,不过在读取缓存的时候依然有这个风险遇到未知类型。
不过any不就是interface{}吗,所以M不就一定是个interface{},如果不是interface不应该会编译错误?如果是int等基础类型不为空应该是预期内的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M
is struct{}
when called from GetTrace()
. any(struct{}{}) == nil
is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it meets your expectations, but checking metadata as nil
randomly would be very strange (e.g. if M
happens to be a thin pointer in the future, isNil
will be false even for nil pointers).
I would prefer fixing this issue at a higher level, e.g. just making the trace.Metadata
field an optional field or a slice, or just correcting it in the reader where it knows the real type of M
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, 我直接在读缓存的地方加个兼容吧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M
isstruct{}
when called fromGetTrace()
.any(struct{}{}) == nil
is false.
不过...这个好像是符合空指针判断逻辑的,这个case就不应该为nil
756f654
to
71760a4
Compare
71760a4
to
e6c150f
Compare
Description
Related issues
Special notes for your reviewer: