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

Fix apdex threshold config bug when dynamic configuration is enabled. #5097

Closed
wants to merge 1 commit into from
Closed

Conversation

Ax1an
Copy link
Member

@Ax1an Ax1an commented Jul 14, 2020

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues


Bug fix

  • Bug description.

    Fix apdex threshold config bug when dynamic configuration is enabled.
    This bug will result in a large number of warn logs, even if the service-apdex-threshold.yml file exist.
    image

  • How to fix?

    Optimize processing logic.
    If there is a core.default.apdexThreshold in the dynamic configuration, use it.
    If there is no core.default.apdexThreshold in the dynamic configuration, the service-apdex-threshold.yml file is read. And it is read only once when the configuration is switched. For example, delete core.default.apdexThreshold in the dynamic configuration.
    Only print the warn log if the service-apdex-threshold.yml file does not exist.


New feature or improvement

  • Describe the details and related test reports.

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #5097 into master will decrease coverage by 0.42%.
The diff coverage is 12.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5097      +/-   ##
============================================
- Coverage     53.09%   52.66%   -0.43%     
+ Complexity     2985     2911      -74     
============================================
  Files          1417     1417              
  Lines         30719    30724       +5     
  Branches       3417     3422       +5     
============================================
- Hits          16310    16182     -128     
- Misses        13605    13738     +133     
  Partials        804      804              
Impacted Files Coverage Δ Complexity Δ
...oap/server/core/analysis/ApdexThresholdConfig.java 54.05% <12.50%> (-11.58%) 4.00 <0.00> (ø)
...ing/oap/server/library/util/ProtoBufJsonUtils.java 0.00% <0.00%> (-90.00%) 0.00% <0.00%> (-2.00%)
...tworkalias/NetworkAddressAliasSetupDispatcher.java 11.11% <0.00%> (-88.89%) 1.00% <0.00%> (-1.00%)
...ler/v8/rest/ManagementServiceKeepAliveHandler.java 32.00% <0.00%> (-64.00%) 2.00% <0.00%> (-1.00%)
...rest/ManagementServiceReportPropertiesHandler.java 27.58% <0.00%> (-62.07%) 2.00% <0.00%> (-2.00%)
...lysis/manual/networkalias/NetworkAddressAlias.java 2.43% <0.00%> (-56.10%) 0.00% <0.00%> (-5.00%)
...p/server/core/source/NetworkAddressAliasSetup.java 33.33% <0.00%> (-50.01%) 2.00% <0.00%> (-1.00%)
...erver/library/client/jdbc/JDBCClientException.java 0.00% <0.00%> (-50.00%) 0.00% <0.00%> (-1.00%)
...r/listener/NetworkAddressAliasMappingListener.java 36.36% <0.00%> (-39.40%) 4.00% <0.00%> (-1.00%)
.../v8/rest/TraceSegmentReportBaseServletHandler.java 42.30% <0.00%> (-30.77%) 2.00% <0.00%> (-2.00%)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5972c2...b5b1199. Read the comment docs.

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Are you trying to fail back to config file? I am not sure whether this is a recommended way. I think other config doesn't follow this. Such as alarm. Could you recheck?

@wu-sheng wu-sheng added the TBD To be decided later, need more discussion or input. label Jul 14, 2020
@wu-sheng
Copy link
Member

Behavior should be consistent acrosee all config items. I think you should fix the dynamic configuration center logic, rather than fixing like this.

@Ax1an Ax1an closed this Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TBD To be decided later, need more discussion or input.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants