Skip to content

Conversation

kklinan
Copy link

@kklinan kklinan commented Jan 26, 2022

Describe what this PR does / why we need it

Does this pull request fix one issue?

Fixes #453

Describe how you did it

Describe how to verify it

Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2022

CLA assistant check
All committers have signed the CLA.

@sczyh30
Copy link
Member

sczyh30 commented Feb 8, 2022

Could you please fix the CI failure?

@sczyh30 sczyh30 added the to-review PRs to review label Feb 8, 2022
Initialize global metric variables
"circuit_breaker_state_changed_total",
"Circuit breaker total state change count",
[]string{"resource", "from_state", "to_state"})
once sync.Once
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no need to use sync.Once

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
Author

Choose a reason for hiding this comment

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

Okay, I'll fix it.

Copy link
Author

Choose a reason for hiding this comment

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

Is it possible to consider putting it inside a separate instance of sentinel instead of a global instance?

Copy link
Collaborator

@binbin0325 binbin0325 left a comment

Choose a reason for hiding this comment

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

CI 处理一下吧,有data race

@kklinan kklinan closed this May 18, 2022
@kklinan
Copy link
Author

kklinan commented May 18, 2022

CI 处理一下吧,有data race

删除 once 导致的,是否不应该删除呢?

@kklinan kklinan reopened this May 18, 2022
@binbin0325
Copy link
Collaborator

CI 处理一下吧,有data race

删除 once 导致的,是否不应该删除呢?

嗯嗯,目前看还是使用once吧

Copy link
Collaborator

@binbin0325 binbin0325 left a comment

Choose a reason for hiding this comment

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

LGTM

@eiopus
Copy link

eiopus commented Dec 8, 2022

这个提交是否还有问题呢,然后什么时候能合并嘞,目前在最新的 [v1.0.4] tag 下还是有这个问题的
@kklinan @binbin0325 @sczyh30

@qshuai
Copy link

qshuai commented Jan 6, 2023

加油,需要这个修复

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-review PRs to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] App name in metric is unknown_ go_ service
6 participants