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.

Sign up for GitHub

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

Jump to bottom

extension mechanism for custom logs and span attributes #1451

Merged
merged 6 commits into from
Dec 5, 2024
Merged

extension mechanism for custom logs and span attributes #1451

merged 6 commits into from
Dec 5, 2024

Conversation

rinfx
Copy link
Collaborator

@rinfx rinfx commented Oct 29, 2024

扩展SDK，支持在插件中向access_log中增加自定义的扩展字段，字段值为custom_log，可以跨插件调用接口对访问日志进行扩展，附加使用demo。

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.54%. Comparing base (ef31e09) to head (c1c397d).
Report is 220 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1451      +/-   ##
==========================================
+ Coverage   35.91%   43.54%   +7.63%     
==========================================
  Files          69       76       +7     
  Lines       11576    12325     +749     
==========================================
+ Hits         4157     5367    +1210     
+ Misses       7104     6624     -480     
- Partials      315      334      +19

see 69 files with indirect coverage changes

CH3CHO
CH3CHO requested changes Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@CH3CHO CH3CHO left a comment

Choose a reason for hiding this comment

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

看没看完，先标注这些

plugins/wasm-go/pkg/wrapper/property_wrapper.go Outdated Show resolved Hide resolved
plugins/wasm-go/pkg/wrapper/property_wrapper.go Outdated Show resolved Hide resolved
plugins/wasm-go/pkg/wrapper/property_wrapper.go Outdated Show resolved Hide resolved
CH3CHO
CH3CHO requested changes Oct 31, 2024
View reviewed changes
plugins/wasm-go/extensions/custom-logs/main.go Outdated Show resolved Hide resolved
plugins/wasm-go/pkg/wrapper/property_wrapper.go Outdated Show resolved Hide resolved
plugins/wasm-go/pkg/wrapper/property_wrapper.go Outdated Show resolved Hide resolved
plugins/wasm-go/pkg/wrapper/property_wrapper.go Outdated
customLog := map[string]interface{}{}
if string(preMarshaledJsonLogStr) != "" {
// e.g. {"feild1":"value1","feild2":"value2"}
preJsonLogStr := UnmarshalStr(fmt.Sprintf(`"%s"`, string(preMarshaledJsonLogStr)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

不太能理解为什么前面 marshal 的时候把首尾的引号去掉，这里又加上。如果之前就没去掉，现在是不是也不用单独加上了？

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

日志的format里面每个字段都会有一个""，所以当某个字段是一个序列化的json字符串的时候，把首位的""去掉让打印到控制台的日志格式是一个合法的json。所以实际上filter_state里面保存的不是一个完整的序列化后的json字符串，在使用json.Unmarshal进行反序列化时需要再把""加上

plugins/wasm-go/pkg/wrapper/property_wrapper.go Outdated Show resolved Hide resolved
johnlanni
johnlanni requested changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@johnlanni johnlanni left a comment

Choose a reason for hiding this comment

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

可以ctx下增加一个 addAccessLogField 的函数，支持用户将自定义日志字段存到 ctx 里，然后在 onDone 阶段统一通过 setProperty 设置进去。这样用户使用简单，整体性能也是最优的。

@rinfx
Copy link
Collaborator Author

rinfx commented Dec 4, 2024

可以ctx下增加一个 addAccessLogField 的函数，支持用户将自定义日志字段存到 ctx 里，然后在 onDone 阶段统一通过 setProperty 设置进去。这样用户使用简单，整体性能也是最优的。

验证了下，onStreamDone里面setProperty不生效

@rinfx rinfx force-pushed the extend_access_log branch from eb417bf to 53718c5 Compare December 4, 2024 03:53
@rinfx rinfx force-pushed the extend_access_log branch from 53718c5 to 51f18e7 Compare December 4, 2024 12:44
@rinfx rinfx force-pushed the extend_access_log branch from 96e91d2 to 750621e Compare December 5, 2024 06:52
@rinfx rinfx changed the title support custom log extension extension mechanism for custom logs and span attributes Dec 5, 2024
johnlanni
johnlanni requested changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@johnlanni johnlanni left a comment

Choose a reason for hiding this comment

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

https://higress.cn/docs/ebook/wasm16/#3-envoy-%E5%B1%9E%E6%80%A7attributes

需要同步更新一下sdk文档，另外 attribute 可能跟 envoy 的 attribute 混淆，叫 userAttribute 可能好一些

johnlanni
johnlanni approved these changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@johnlanni johnlanni left a comment

Choose a reason for hiding this comment

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

LGTM

CH3CHO
CH3CHO approved these changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@CH3CHO CH3CHO left a comment

Choose a reason for hiding this comment

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

OK

@CH3CHO CH3CHO merged commit 4332273 into alibaba:main Dec 5, 2024
13 checks passed
@rinfx rinfx mentioned this pull request Dec 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnlanni johnlanni johnlanni approved these changes

@CH3CHO CH3CHO CH3CHO approved these changes

@WeixinX WeixinX Awaiting requested review from WeixinX

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rinfx @codecov-commenter @CH3CHO @johnlanni