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

[AWS][RDS] Add metric type to fields #6094

Merged
merged 9 commits into from Jun 2, 2023

Conversation

constanca-m
Copy link
Contributor

What does this PR do?

Add metric type to fields of RDS data stream, necessary to support TSDB in the future.

Details

Gauge and counter fields were defined based on:

  1. The metrics that represent a specific value in time, without consideration for the previous one, are gauge.
  2. If a metric keeps increment and decrementing, it is a gauge.
  3. If a metric is cumulative, then it is a counter - but it can only increment.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

Refer to #6078

Related issues

constanca-m and others added 3 commits May 4, 2023 16:48
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
@constanca-m constanca-m mentioned this pull request May 4, 2023
6 tasks
@elasticmachine
Copy link

elasticmachine commented May 4, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-06-01T16:22:29.472+0000

  • Duration: 55 min 45 sec

Test stats 🧪

Test Results
Failed 0
Passed 188
Skipped 4
Total 192

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented May 4, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (15/15) 💚
Files 93.75% (15/16)
Classes 93.75% (15/16)
Methods 86.131% (236/274)
Lines 85.925% (7387/8597)
Conditionals 100.0% (0/0) 💚

@@ -25,321 +25,422 @@
- name: rds
type: group
fields:
- name: cpu.total.pct
Copy link
Contributor

Choose a reason for hiding this comment

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

this format is not working? any specific reason to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The format work, but the file has many fields and it was easier to group them and made changes this way @tetianakravchenko

type: group
fields:
- name: replicated_write_io.bytes
metric_type: counter
Copy link
Contributor

Choose a reason for hiding this comment

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

so this value is not an aggregated value during some period of time, similar to other metrics? I thought cloudwatch always provides aggregated values

the same question regarding the data_transfer.bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with these metrics is that are only considered for a specific time window, like 15 minutes. So if we receive 2 documents, there is no guarantee that the most recent document will have a higher value for that field than the other. @tetianakravchenko

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And since we only receive a document after that time window (I think), there is no point in using counter since we never have values that relate to each other @tetianakravchenko

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it then be a gauge? instead of counter? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was making a case for gauge, I thought I had set it as such. But now I am confused on why I set this metric as a counter in the first place 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it @tetianakravchenko to be a gauge

description: |
The amount of network throughput both received from and transmitted to clients by each instance in the Aurora MySQL DB cluster, in bytes per second.
- name: network_receive
metric_type: gauge
Copy link
Contributor

Choose a reason for hiding this comment

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

if it is not an aggregated data, should it be counter then?

then same regarding network_transmit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same case as the other. Practical example of a visualization for one specific instance:
image

constanca-m and others added 5 commits June 1, 2023 18:07
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
type: long
format: bytes
description: |
The amount of available storage space.
- name: maximum_used_transaction_ids
metric_type: counter
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please double-check if this should be a counter? sounds reasonable, based on the description, but just in case

type: long
description: |
The percentage of requests that are served by the Resultset cache.
- name: engine_uptime.sec
metric_type: counter
Copy link
Contributor

Choose a reason for hiding this comment

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

the same here: could you please double-check if this should be a counter? sounds reasonable, but just in case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I checked yesterday and I believe both are counter. @tetianakravchenko

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of this metric (this visualizations shows the difference between the last value and the new value):
image

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, strange to see this spike between 13:30 and 14:30, if this metric is suppose to monotonically increase, no? and why later value is not increasing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are increasing @tetianakravchenko The difference between the previous value and the current value is always >0. I expressed myself wrongly, sorry for that. We see a huge spike because we received no values for some time (see the same visualization, this time including the empty rows):
image
So when we received the value at 14:xxh, the last one we had was the one from 13:xx, so there was a bigger difference.

@constanca-m constanca-m merged commit cdb74ef into elastic:main Jun 2, 2023
3 checks passed
@constanca-m constanca-m deleted the rds-add-metrics branch June 2, 2023 08:44
@elasticmachine
Copy link

Package aws - 1.38.1 containing this change is available at https://epr.elastic.co/search?package=aws

sodhikirti07 pushed a commit that referenced this pull request Jun 15, 2023
* Add metric type.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants