-
Notifications
You must be signed in to change notification settings - Fork 470
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
[frontend-gray] Increase gray types according to the ratio-weight gray #1291
base: main
Are you sure you want to change the base?
Conversation
feat: 🎸 frontend-gray plugin support cdn type deploy
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1291 +/- ##
==========================================
+ Coverage 35.91% 44.31% +8.40%
==========================================
Files 69 75 +6
Lines 11576 9823 -1753
==========================================
+ Hits 4157 4353 +196
+ Misses 7104 5142 -1962
- Partials 315 328 +13 |
return grayConfig.BaseDeployment | ||
} | ||
|
||
// 确保每个用户每次访问的都是走同一版本 |
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.
debug情况就不做用户粘滞咯?
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.
debug情况就不做用户粘滞咯?
是的是的,所以这个debug,命名为 debugGrayWeight
了
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.
不是太理解什么时候应该用这个功能。。。
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.
不是太理解什么时候应该用这个功能。。。
这个不算是功能,只是让用户进行观测效果。否则看不出按比率灰度的效果。但是这个透出确实有点奇怪。没有想到好的方式,有什么推荐的建议吗
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.
要不你加一下我钉钉 CH3CHO,我们单独沟通一下。
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.
让用户进行观测效果:观测什么的效果呢?
- 如果是灰度真实用户,不做粘滞可能会导致用户体验不一致。
- 如果是开发人员去验证灰度版本,那他自己种一个灰度Cookie就可以了吧?
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.
CH3CHO
好的好,这个debug模式 确实可以删除的,已经删除哈
NotFound = "not-found" | ||
IsNotFound = "is-not-found" | ||
// 2 days | ||
MaxAgeCookie = "172800" |
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.
这个就是让灰度的粘滞保持多长时间吧?是不是最好支持用户配置?
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.
这个就是让灰度的粘滞保持多长时间吧?是不是最好支持用户配置?
通过配置 userStickyMaxAge
字段支持,默认为172800,2天
@@ -19,7 +20,9 @@ import ( | |||
|
|||
func LogInfof(format string, args ...interface{}) { | |||
format = fmt.Sprintf("[%s] %s", "frontend-gray", format) | |||
proxywasm.LogInfof(format, args...) | |||
if os.Getenv("TEST_MODE") != "" { |
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.
- 是不是可以在外面把 env get 好并做好判定设置为 bool,这样就不需要每次单独读取和判定了?
- 如果不是 TEST_MODE,上面的 Sprintf 是不是也没必要调用了?
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.
- 是不是可以在外面把 env get 好并做好判定设置为 bool,这样就不需要每次单独读取和判定了?
- 如果不是 TEST_MODE,上面的 Sprintf 是不是也没必要调用了?
是的,这个 debug模式也删除了哈
config.JsonToGrayConfig(gjson.Parse(test.input), grayConfig) | ||
reslut := FilterGrayWeight(grayConfig) | ||
reslut := FilterGrayWeight(grayConfig, []string{"base", "1.0.1"}, "192.168.1.1") |
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.
result 拼写错误
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.
result 拼写错误
fixed
config.JsonToGrayConfig(gjson.Parse(test.input), grayConfig) | ||
reslut := FilterGrayWeight(grayConfig) | ||
reslut := FilterGrayWeight(grayConfig, []string{"base", "1.0.1"}, "192.168.1.1") | ||
t.Logf("reslut-----: %v", reslut) |
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.
同上,拼写错误
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.
同上,拼写错误
fixed
@@ -11,7 +11,9 @@ const ( | |||
XForwardedFor = "x-forwarded-for" | |||
XPreHigressTag = "x-pre-higress-tag" | |||
IsIndex = "is-index" |
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.
isIndex这个名字是不是换一下,改成isPageRequest之类的?
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.
isIndex这个名字是不是换一下,改成isPageRequest之类的?
fixed
@@ -119,8 +119,7 @@ var indexSuffixes = []string{ | |||
".html", ".htm", ".jsp", ".php", ".asp", ".aspx", ".erb", ".ejs", ".twig", | |||
} | |||
|
|||
// IsIndexRequest determines if the request is an index request | |||
func IsIndexRequest(fetchMode string, p string) bool { | |||
func GetIsPageRequest(fetchMode string, p string) bool { |
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.
建议是把 Get 去掉,直接就叫 IsPageRequest。另外这个 p 的参数名不太好理解。如果是怕和包名冲突的话,可以考虑把 import 那个 path 起个别名叫 gopath。这个变量还是叫 path。
// 用户粘滞,确保每个用户每次访问的都是走同一版本 | ||
if len(preVersions) > 1 && preVersions[1] != "" && uniqueClientId == preVersions[1] { | ||
for _, deployment := range deployments { | ||
if deployment.Version == strings.Trim(preVersions[0], " ") { |
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.
Trim操作做一次是不是就行了?
deployments := append(grayConfig.GrayDeployments, grayConfig.BaseDeployment) | ||
LogInfof("uniqueClientId: %s, preVersions: %v", uniqueClientId, preVersions) | ||
// 用户粘滞,确保每个用户每次访问的都是走同一版本 | ||
if len(preVersions) > 1 && preVersions[1] != "" && uniqueClientId == preVersions[1] { |
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.
preversion的生成和解析建议抽一个方法并返回一个有语义的结构,现在写Cookie、读Cookie和用这个值分散在不同的地方,不便于理解和管理。
@@ -18,12 +18,12 @@ description: 前端灰度插件配置参考 | |||
|----------------|--------------|----|-----|----------------------------------------------------------------------------------------------------| | |||
| `grayKey` | string | 非必填 | - | 用户ID的唯一标识,可以来自Cookie或者Header中,比如 userid,如果没有填写则使用`rules[].grayTagKey`和`rules[].grayTagValue`过滤灰度规则 | | |||
| `graySubKey` | string | 非必填 | - | 用户身份信息可能以JSON形式透出,比如:`userInfo:{ userCode:"001" }`,当前例子`graySubKey`取值为`userCode` | | |||
| `userStickyMaxAge` | string | 非必填 | 172800 | 用户粘滞的时长:单位为秒,默认为`172800`,2天时间 | |
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.
这个字段是不是用 int 类型更好一点?
ctx.SetContext(config.IsIndex, isIndex) | ||
ctx.SetContext(config.XForwardedFor, xForwardedFor) | ||
ctx.SetContext(config.IsPageRequest, isPageRequest) | ||
ctx.SetContext(config.XUniqueClient, uniqueClientId) |
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.
存的是ID,这个常量是不是也叫XUniqueClientId好一点?
if grayConfig.Rewrite != nil && grayConfig.Rewrite.Host != "" { | ||
// 删除Content-Disposition,避免自动下载文件 | ||
proxywasm.RemoveHttpResponseHeader("Content-Disposition") | ||
} | ||
|
||
isIndex := ctx.GetContext(config.IsIndex).(bool) | ||
isPageRequest := ctx.GetContext(config.IsPageRequest).(bool) |
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.
这些地方建议都用两个返回值的 cast 方式,避免 panic。
Ⅰ. Describe what this PR did
灰度规则
beta-user
的灰度范围为 50%可以分别在 header,body的首部
first
和尾部last
注入相关代码。Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews