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

Refine member metrics configuration #15818

Merged

Conversation

@blazember
Copy link
Contributor

blazember commented Oct 18, 2019

This change refines the member-side metrics configuration to address two
issues with the previously added config:

  • It was not clear that retention-seconds was for MC only
  • Further improving the MC and JMX configs was not supported by the
    attributes

To address these, the configuration changes as follows.

Before this change:

<metrics enabled="true" mc-enabled="true" jmx-enabled="true">
    <collection-interval-seconds>5</collection-interval-seconds>
    <retention-seconds>5</retention-seconds>
    <metrics-for-data-structures>false</metrics-for-data-structures>
    <minimum-level>INFO</minimum-level>
</metrics>

After this change:

<metrics enabled="true">
    <management-center enabled="true">
        <retention-seconds>5</retention-seconds>
    </management-center>
    <jmx enabled="true"/>
    <collection-frequency-seconds>5</collection-frequency-seconds>
</metrics>

The programmatic and YAML configuration follows the same structure. With the defaults above.

The metrics-for-data-structures and level settings are removed from
the configuration and are configurable with the following boolean system
properties only:

  • hazelcast.metrics.datastructures.enabled: to disable collecting
    datastructure metrics, enabled by default
  • hazelcast.metrics.debug.enabled: to collect DEBUG level metrics

MANDATORY level can no longer be configurable. These settings were
added to the 3.x line to better control the memory footprint of the collection
system. Since in the 4.x line the vast majority of the metrics are discovered
dynamically, the footprint is almost constant. What might change is the size
of the compressed blob sent to MC (if enabled) and the JMX beans (if enabled).
The system properties are kept as options to further tune the footprint without
disabling the metrics system entirely.

It is also a requirement that all metrics configuration should be
overrideable via system properties. This is also implemented in this
change by overriding the configuration from the properties during the
Node instance's creation. The concrete system properties that override
the configuration fields are added to the javadocs of the metrics
config classes and to the xsd too.

Besides refining the changes above, the following gaps were also closed:

  • Improve javadoc and xsd documentation for the metrics config elements
  • Add Spring XML configuration
  • Add config comments to hazelcast-full-example.[xml|yaml]
  • Add metrics config to hazelcast-fullconfig.[xml|yaml]
  • Add metrics metrics config to hazelcast-default.[xml|yaml]

EE PR: hazelcast/hazelcast-enterprise#3259

@blazember blazember added this to the 4.0 milestone Oct 18, 2019
@blazember blazember self-assigned this Oct 18, 2019
@blazember blazember force-pushed the blazember:4.0/mc-metrics/config-refinement branch from 16d0035 to 92a3595 Oct 18, 2019
@blazember blazember marked this pull request as ready for review Oct 18, 2019
@blazember blazember requested a review from hazelcast/clients as a code owner Oct 18, 2019
@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Oct 18, 2019

@sancar
sancar approved these changes Oct 22, 2019
Copy link
Member

sancar left a comment

Ok for the client-side.

@blazember blazember force-pushed the blazember:4.0/mc-metrics/config-refinement branch from 92a3595 to 43b6f43 Nov 11, 2019
@blazember blazember force-pushed the blazember:4.0/mc-metrics/config-refinement branch 4 times, most recently from fd1f4c9 to 349117b Nov 11, 2019
@hazelcast hazelcast deleted a comment from blazember Nov 13, 2019
@hazelcast hazelcast deleted a comment from blazember Nov 13, 2019
@hazelcast hazelcast deleted a comment from blazember Nov 13, 2019
@hazelcast hazelcast deleted a comment from blazember Nov 13, 2019
@blazember blazember force-pushed the blazember:4.0/mc-metrics/config-refinement branch 2 times, most recently from 3a4933c to bee3d17 Nov 13, 2019
@blazember blazember requested a review from puzpuzpuz Nov 14, 2019
Copy link
Contributor

puzpuzpuz left a comment

LGTM. Left several minor comments to discuss/address.

blazember added 4 commits Oct 17, 2019
This change refines the member-side metrics configuration to address two
issues with the previously added config:
- It was not clear that `retention-seconds` was for MC only
- Further improving the MC and JMX configs was not supported by the
attributes

To address these, the configuration changes as follows.

Before this change:
```
<metrics enabled="false" mc-enabled="false" jmx-enabled="false">
    <collection-interval-seconds>10</collection-interval-seconds>
    <retention-seconds>30</retention-seconds>
    <metrics-for-data-structures>true</metrics-for-data-structures>
    <minimum-level>DEBUG</minimum-level>
</metrics>
```

After this change:
```
<metrics enabled="false">
    <management-center enabled="false">
        <retention-seconds>30</retention-seconds>
    </management-center>
    <jmx enabled="false"/>
    <collection-frequency-seconds>10</collection-frequency-seconds>
    <data-structure-metrics-enabled>true</data-structure-metrics-enabled>
    <level>DEBUG</level>
</metrics>
```

The programmatic and YAML configuration follows the same structure.

It is also a requirement that all metrics configurations should
overrideable via system properties. This is also implemented in this
change by overriding the configuration from the properties during the
`Node` instances is created. The concrete system properties are added to
the javadocs of the metrics config classes and to the xsd too.

Besides refining the changes above, the following gaps were also closed:
- Extends javadoc and xsd documentation for the metrics config elements
- Add Spring XML configuration
- Add full-example config comments
- Add metrics config to hazelcast-fullconfig.[xml|yaml]
- Add metrics metrics config to hazelcast-default.[xml|yaml]
@blazember blazember force-pushed the blazember:4.0/mc-metrics/config-refinement branch from ab731b6 to 9c68e55 Nov 15, 2019
Copy link
Contributor

tkountis left a comment

LGTM


@Override
public final int hashCode() {
return (enabled ? 1 : 0);

This comment has been minimized.

Copy link
@tkountis

tkountis Nov 17, 2019

Contributor

Why not Boolean.hashCode(...)? not a big concern here, but larger figures (and primes) would probably be more fair towards hashing structures.

This comment has been minimized.

Copy link
@blazember

blazember Nov 18, 2019

Author Contributor

Thank you @tkountis, updated with Boolean.hashCode(enabled) here and in the other two metrics config classes as well.

@blazember blazember force-pushed the blazember:4.0/mc-metrics/config-refinement branch from 0d99eed to 1c0c822 Nov 18, 2019
@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Nov 18, 2019

run-lab-run

@blazember blazember merged commit 711cad9 into hazelcast:master Nov 18, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@blazember blazember deleted the blazember:4.0/mc-metrics/config-refinement branch Nov 18, 2019
@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Nov 18, 2019

Thank you a lot for your review @puzpuzpuz @sancar @tkountis 👍

petrpleshachkov pushed a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 22, 2019
This change refines the member-side metrics configuration to address two
issues with the previously added config:
- It was not clear that `retention-seconds` was for MC only
- Further improving the MC and JMX configs was not supported by the
attributes

To address these, the configuration changes as follows.

Before this change:
```
<metrics enabled="true" mc-enabled="true" jmx-enabled="true">
    <collection-interval-seconds>5</collection-interval-seconds>
    <retention-seconds>5</retention-seconds>
    <metrics-for-data-structures>false</metrics-for-data-structures>
    <minimum-level>INFO</minimum-level>
</metrics>
```

After this change:
```
<metrics enabled="true">
    <management-center enabled="true">
        <retention-seconds>5</retention-seconds>
    </management-center>
    <jmx enabled="true"/>
    <collection-frequency-seconds>5</collection-frequency-seconds>
</metrics>
```

The programmatic and YAML configuration follows the same structure. With the defaults above. 

The `metrics-for-data-structures` and `level` settings are removed from 
the configuration and are configurable with the following boolean system 
properties only:
- `hazelcast.metrics.datastructures.enabled`: to disable collecting 
datastructure metrics, enabled by default
- `hazelcast.metrics.debug.enabled`: to collect `DEBUG` level metrics

`MANDATORY` level can no longer be configurable. These settings were 
added to the 3.x line to better control the memory footprint of the collection 
system. Since in the 4.x line the vast majority of the metrics are discovered 
dynamically, the footprint is almost constant. What might change is the size 
of the compressed blob sent to MC (if enabled) and the JMX beans (if enabled). 
The system properties are kept as options to further tune the footprint without 
disabling the metrics system entirely. 

It is also a requirement that all metrics configuration should be
overrideable via system properties. This is also implemented in this
change by overriding the configuration from the properties during the
`Node` instance's creation. The concrete system properties that override
the configuration fields are added to the javadocs of the metrics 
config classes and to the xsd too.

Besides refining the changes above, the following gaps were also closed:
- Improve javadoc and xsd documentation for the metrics config elements
- Add Spring XML configuration
- Add config comments to `hazelcast-full-example.[xml|yaml]`
- Add metrics config to `hazelcast-fullconfig.[xml|yaml]`
- Add metrics metrics config to `hazelcast-default.[xml|yaml]`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.