-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[receiver/bigip] Add Big-IP Metrics Receiver #9893
Conversation
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.
I think this also needs an entry in the CODEOWNERS file, as well
receiver/bigipreceiver/testdata/integration/mock_server/record_big_ip_responses.sh
Outdated
Show resolved
Hide resolved
receiver/bigipreceiver/testdata/integration/mock_server/record_big_ip_responses.sh
Show resolved
Hide resolved
receiver/bigipreceiver/testdata/apiresponses/get_nodes_stats_response.json
Outdated
Show resolved
Hide resolved
receiver/bigipreceiver/testdata/apiresponses/get_pool_members_stats_response_1.json
Outdated
Show resolved
Hide resolved
c95b61f
to
71f521b
Compare
71f521b
to
27e10bd
Compare
d1532f5
to
373da2a
Compare
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.
Overall looks very good. A few things though
receiver/bigipreceiver/metadata.yaml
Outdated
bigip.pool.member.count: | ||
description: Total number of pool members. | ||
unit: "{members}" | ||
sum: | ||
monotonic: false | ||
aggregation: cumulative | ||
value_type: int | ||
enabled: true | ||
bigip.pool.active_member.count: | ||
description: Number of active pool members. | ||
unit: "{members}" | ||
sum: | ||
monotonic: false | ||
aggregation: cumulative | ||
value_type: int | ||
enabled: true | ||
bigip.pool.available_member.count: | ||
description: Number of available pool members. | ||
unit: "{members}" | ||
sum: | ||
monotonic: false | ||
aggregation: cumulative | ||
value_type: int | ||
enabled: true |
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.
Does "active + available = total"?
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.
@djaglowski no, active is more about available + enabled i believe
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.
Is there a reasonable way to combine all this? I don't know exactly how they relate to each other, but it seems like bigip.pool.member.count
could just have one or more attributes to differentiate the data points.
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.
@djaglowski so we had a discussion in slack, and it sounds like maybe these metrics don't belong lumped into one. they are definitely related, but they don't individually add together to make anything meaningful.
total = total amount of pools
available = available amount of pools (pools that appear to have valid configurations and "could" be used)
active = subset of available pools. the pools that are both available AND enabled.
so when you look at active pools and you look at available pools, you might be looking at the same pools (at least for some of them). let me know if you still think strongly that we should lump these together even if the different groups dont' add up to a total
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.
I think this is a good example of exactly where aggregation plays a key role in metric design.
you might be looking at the same pools
Whenever there are data points that meaningfully overlap, we need to try to "name" the overlapping set. Then we can determine the right model.
From what you've told me, I understand that total = available (valid config) + unavailable (invalid config)
and available = active (valid config & enabled) + inactive (valid config & disabled)
. So, at at face value, we have at least one way to represent the total as a sum of parts, with no overlap: total = active + inactive + unavailable
.
However, this does not provide a way to represent "available" as a distinct value, so we need to look at incorporating a second attribute that would allow us to aggregate all available pools. We can have two attributes:
config_state: [valid | invalid]
: Whether or not the pool appears to have a valid configenabled_state [enabled | disabled]
: Whether or not the pool is enabled
In this representation we still have the three data points, active + inactive + unavailable
:
active: valid & enabled
inactive: valid & disabled
unavailable: invalid & disabled
From the three data points aggregation provides meaningful totals:
- Grouping by
config_state
leaves available vs unavailable. - Grouping by
enabled_state
leaves active vs inactive. - Grouping by both dimensions leaves the overall total.
Why do it this way? Because the data model is designed to model the correlation between related data points and to give users the ability to group the data along meaningful dimensions. With this representation, we are providing a lot more information than with three separate metrics that overlap each other. The relation between the data points is built into the model and the user effectively has five related data points to work with (active
/inactive
, available
/unavailable
, total
)
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.
@djaglowski changed these metrics to be a single one with an active attribute ("active"/"inactive"). removed available count as it doesn't seem to be as useful
Also, note the failing check needs addressing: |
0ce1d0c
to
1be1cad
Compare
1be1cad
to
cb84f20
Compare
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.
Thanks @StefanKurek. Looks good
Adds Big-IP Metric Receiver
Description:
Add Big-IP Metrics receiver. Adds a client, factory, config, and scraper to accomplish this.
Link to tracking Issue:
#9680
Testing:
Added config, client, factory, and scraper unit tests to validate new code.
An integration test has been added as well which spins up a mock HTTP server which plays back real responses form a Big-IP environment. A script has been included to help future developers record the necessary responses from their own Big-IP environments to work with the mock REST Server.
Documentation:
Added doc explaining use of receiver.