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 issue62 : Support customized log directory and configuration properties directory #804

Merged
merged 31 commits into from
Jul 8, 2019
Merged

Fix issue62 : Support customized log directory and configuration properties directory #804

merged 31 commits into from
Jul 8, 2019

Conversation

linlinisme
Copy link
Collaborator

@linlinisme linlinisme commented May 31, 2019

Does this pull request fix one issue?

Resolves #62

Describe how you did it

Split the configuration into two parts: the log and the sentinel-core. There are special tool classes to load the respective configurations. The tool class supports reading specified files, default files, and virtual machine parameter configurations.

the sentinel-core support configurations :

    public static final String APP_TYPE = "csp.sentinel.app.type";
    public static final String CHARSET = "csp.sentinel.charset";
    public static final String SINGLE_METRIC_FILE_SIZE = "csp.sentinel.metric.file.single.size";
    public static final String TOTAL_METRIC_FILE_COUNT = "csp.sentinel.metric.file.total.count";
    public static final String COLD_FACTOR = "csp.sentinel.flow.cold.factor";
    public static final String STATISTIC_MAX_RT = "csp.sentinel.statistic.max.rt";

Customize the loaded configuration file by configuring the csp.sentinel.config.file property.
for example -Dcsp.sentinel.config.file=sentinel.properties

the log:

    public static final String LOG_DIR = "csp.sentinel.log.dir";
    public static final String LOG_NAME_USE_PID = "csp.sentinel.log.use.pid";
    public static final String LOG_OUTPUT_TYPE = "csp.sentinel.log.output.type";
    public static final String LOG_CHARSET = "csp.sentinel.log.charset";

Customize the loaded configuration file by configuring the csp.sentinel.config.file property.
for example -Dcsp.sentinel.config.file=sentinel.properties

Pay attention! the log and sentinel-core configuration share a same config file now

Configure effective priority: virtual machine parameters -> -Dcsp.sentinel.config.file specifies file configuration

If -Dcsp.sentinel.config.file is not specified, the default value is sentinel.properties. That means you only need to add sentinel.properties to your project then you can load the custom configuration.

If you are the old version of the specified directory configuration: ${user.home}/logs/csp/appName.properties
And you want to keep the existing configuration path, it is also ok. You don't need to configure -Dcsp.sentinel.config.file nor need to add sentinel.properties to your project. Because it provides legacy compatibility.

Describe how to verify it

Configure the attribute value and run the project to see if it works.
And you can run the test cases in SentinelConfigTestLogBaseTestConfigUtilTest

@codecov-io
Copy link

codecov-io commented May 31, 2019

Codecov Report

Merging #804 into master will increase coverage by 0.36%.
The diff coverage is 72.79%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #804      +/-   ##
============================================
+ Coverage     42.25%   42.62%   +0.36%     
- Complexity     1419     1439      +20     
============================================
  Files           307      310       +3     
  Lines          8894     8979      +85     
  Branches       1203     1218      +15     
============================================
+ Hits           3758     3827      +69     
- Misses         4677     4684       +7     
- Partials        459      468       +9
Impacted Files Coverage Δ Complexity Δ
...ava/com/alibaba/csp/sentinel/util/AppNameUtil.java 43.47% <ø> (ø) 5 <0> (ø) ⬇️
...om/alibaba/csp/sentinel/config/SentinelConfig.java 57.74% <100%> (+6.13%) 14 <3> (ø) ⬇️
...baba/csp/sentinel/config/SentinelConfigLoader.java 67.85% <67.85%> (ø) 4 <4> (?)
...ain/java/com/alibaba/csp/sentinel/log/LogBase.java 56.94% <69.56%> (-1.63%) 10 <4> (-1)
.../com/alibaba/csp/sentinel/log/LogConfigLoader.java 71.42% <71.42%> (ø) 4 <4> (?)
...java/com/alibaba/csp/sentinel/util/ConfigUtil.java 73.21% <73.21%> (ø) 12 <12> (?)
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java 100% <0%> (+4.76%) 8% <0%> (+1%) ⬆️

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 c14e329...c71759f. Read the comment docs.

@sczyh30 sczyh30 added to-review To review area/configuration Issues or PRs related to configurations of Sentinel area/logging Issues or PRs related to logging of Sentinel labels Jun 1, 2019
@vitrun
Copy link

vitrun commented Jun 17, 2019

when will this be fixed ? we can not use it in production if custom configuration is unavailable.

@sczyh30
Copy link
Member

sczyh30 commented Jun 17, 2019

when will this be fixed ? we can not use it in production if custom configuration is unavailable.

@vitrun I'll take a review these days. It's expected to be included in 1.7.0 (perhaps in the next few weeks). If you're using Spring Boot/Spring Cloud you could leverage Spring Cloud Alibaba to support configuring via application.properties: https://github.com/spring-cloud-incubator/spring-cloud-alibaba/wiki/Sentinel#more

@vitrun

This comment has been minimized.

@sczyh30

This comment has been minimized.

@linlinisme
Copy link
Collaborator Author

add test cases

@linlinisme
Copy link
Collaborator Author

linlinisme commented Jul 1, 2019

feature support : retrieve properties from relative file.
now, there is three config ways:

  1. absolute path
    -Dcsp.sentinel.config.file=/user/application/config/sentinel.properties
  2. class path
    -Dcsp.sentinel.config.file=classpath:sentinel.properties
  3. relative path
    -Dcsp.sentinel.config.file=config/sentinel.properties
    What need to be attention is the relative root path is the user.dir. For example if you start app in the /user/application dir and you config file is /user/application/config/sentinel.properties
    the correct config way is -Dcsp.sentinel.config.file=config/sentinel.properties

Dear @sczyh30 would you help me view the code?

@linlinisme
Copy link
Collaborator Author

friendly ping :)

@sczyh30
Copy link
Member

sczyh30 commented Jul 5, 2019

friendly ping :)

I'll review these days :)

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit ff33de1 into alibaba:master Jul 8, 2019
@sczyh30
Copy link
Member

sczyh30 commented Jul 8, 2019

Excellent, thanks for contributing!

Just one more thing to consider: it would be convenient if we could also support setting appName (project.name) in the properties file. This might entail discarding the legacy logic (~/logs/csp/${appName}.properties) and should be discussed later. See #239

@sczyh30 sczyh30 removed the to-review To review label Jul 8, 2019
@sczyh30 sczyh30 added this to the 1.7.0 milestone Jul 8, 2019
CST11021 pushed a commit to CST11021/Sentinel that referenced this pull request Nov 3, 2021
* add english document of transaction message design

* Format the style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration Issues or PRs related to configurations of Sentinel area/logging Issues or PRs related to logging of Sentinel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support customized log directory and configuration properties directory
4 participants