[AWS] [S3] fix: improve object size metric calculation #41755

Kavindu-Dodan · 2024-11-22T19:20:10Z

Proposed commit message

This fixes a bug with Cloudflare logpush integration, which uses the S3 input.

#40775 introduced a feature introducing s3_object_size_in_bytes histogram. Internally it utilized S3 SDK's GetObjectOutput instances' ContentLength property ¹. In code level (SDK internally), this is derived using Content-Length header ².

It seems that Cloudflare R2 (API compatible with S3) could omit Content-Length header ³. Hence, I have improved how we derive the s3_object_size_in_bytes metric by avoiding using Content-Length backed method.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Build filebeat from this branch
Enable monitoring endpoints
Configure buckets with data and observe metrics in monitoring endpoint

Screenshots

See metric comparison with proposed collection method (before removing content length usage). Tracked total bytes are same as content length.

Handling validated for gzip

Handling validated for plain text

elasticmachine · 2024-11-22T19:20:12Z

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

x-pack/filebeat/input/awss3/metrics.go

andrewkroh · 2024-11-22T22:08:01Z

x-pack/filebeat/input/awss3/metrics.go

 	totalBytesRead *monitoring.Uint
+
+	trackBytes int64


I think the naming could be more clear. "trackBytes" does not mean much to me. Maybe...

totalBytesReadMetric *monitoring.Uint totalBytesRead int64

perhaps with some godoc for each field.

Done, please see - 91c7d87

I have renamed and added doc comments so the tracking is clear :)

andrewkroh · 2024-11-22T22:10:02Z

x-pack/filebeat/input/awss3/metrics.go

 	return n, err
 }
+
+func (m *monitoredReader) GetTrackedBytes() int64 {


The accessor seems unnecessary. The caller can read straight from the field.

I have removed accessor :)

gizas · 2024-11-25T09:51:09Z

x-pack/filebeat/input/awss3/metrics.go

@@ -219,5 +221,10 @@ func newMonitoredReader(r io.Reader, metric *monitoring.Uint) *monitoredReader {
 func (m *monitoredReader) Read(p []byte) (int, error) {
 	n, err := m.reader.Read(p)
 	m.totalBytesRead.Add(uint64(n))
+	m.trackBytes += int64(n)


What is the diff between totalBytesRead vs trackBytes?

@gizas good point. I added doc comment please see here - 91c7d87#diff-f8abd76e8ccf8f3244f2153161d307bdfd220d7d8b691423bb79fc43cc4a94eaR209-R214

This (renamed to totalBytesReadCurrent) tracks bytes read for currently tracked object. On the other hand, totalBytesRead (renamed to totalBytesReadMetric) track all byes read for all tracked objects. This is a cumulative counter.

gizas

Approving as LGTM the logic!
I agree accessor GetTrackedBytes() can be ommited

Signed-off-by: Kavindu Dodanduwa <[email protected]>

* rely on monitored reader for content length Signed-off-by: Kavindu Dodanduwa <[email protected]> * add tests to validate metrics Signed-off-by: Kavindu Dodanduwa <[email protected]> * add changelog entry Signed-off-by: Kavindu Dodanduwa <[email protected]> * fix lint - ignore ok values as we know the stored value type Signed-off-by: Kavindu Dodanduwa <[email protected]> * review change - rename and accessor Signed-off-by: Kavindu Dodanduwa <[email protected]> --------- Signed-off-by: Kavindu Dodanduwa <[email protected]> (cherry picked from commit 92bb2c5)

…lculation (#41777) * [AWS] [S3] fix: improve object size metric calculation (#41755) * rely on monitored reader for content length Signed-off-by: Kavindu Dodanduwa <[email protected]> * add tests to validate metrics Signed-off-by: Kavindu Dodanduwa <[email protected]> * add changelog entry Signed-off-by: Kavindu Dodanduwa <[email protected]> * fix lint - ignore ok values as we know the stored value type Signed-off-by: Kavindu Dodanduwa <[email protected]> * review change - rename and accessor Signed-off-by: Kavindu Dodanduwa <[email protected]> --------- Signed-off-by: Kavindu Dodanduwa <[email protected]> (cherry picked from commit 92bb2c5) * Update CHANGELOG.next.asciidoc Remove unwanted backport entry --------- Co-authored-by: Kavindu Dodanduwa <[email protected]>

* rely on monitored reader for content length Signed-off-by: Kavindu Dodanduwa <[email protected]> * add tests to validate metrics Signed-off-by: Kavindu Dodanduwa <[email protected]> * add changelog entry Signed-off-by: Kavindu Dodanduwa <[email protected]> * fix lint - ignore ok values as we know the stored value type Signed-off-by: Kavindu Dodanduwa <[email protected]> * review change - rename and accessor Signed-off-by: Kavindu Dodanduwa <[email protected]> --------- Signed-off-by: Kavindu Dodanduwa <[email protected]> (cherry picked from commit 92bb2c5) Co-authored-by: Kavindu Dodanduwa <[email protected]>

Kavindu-Dodan added bug Team:obs-ds-hosted-services Label for the Observability Hosted Services team backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify labels Nov 22, 2024

Kavindu-Dodan requested a review from a team November 22, 2024 19:20

Kavindu-Dodan requested a review from a team as a code owner November 22, 2024 19:20

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 22, 2024

mergify bot assigned Kavindu-Dodan Nov 22, 2024

Kavindu-Dodan force-pushed the fix/improve-object-size-metric-calculation branch from 78c0c74 to 04531f3 Compare November 22, 2024 19:47

Kavindu-Dodan commented Nov 22, 2024

View reviewed changes

x-pack/filebeat/input/awss3/metrics.go Show resolved Hide resolved

andrewkroh reviewed Nov 22, 2024

View reviewed changes

gizas reviewed Nov 25, 2024

View reviewed changes

gizas approved these changes Nov 25, 2024

View reviewed changes

Kavindu-Dodan added 4 commits November 25, 2024 07:35

rely on monitored reader for content length

47f58ab

Signed-off-by: Kavindu Dodanduwa <[email protected]>

add tests to validate metrics

d3998e6

Signed-off-by: Kavindu Dodanduwa <[email protected]>

add changelog entry

e87b6a5

Signed-off-by: Kavindu Dodanduwa <[email protected]>

fix lint - ignore ok values as we know the stored value type

5e496fd

Signed-off-by: Kavindu Dodanduwa <[email protected]>

Kavindu-Dodan force-pushed the fix/improve-object-size-metric-calculation branch from 3a1b4de to 25ea16c Compare November 25, 2024 15:42

Kavindu-Dodan requested a review from andrewkroh November 25, 2024 15:42

andrewkroh approved these changes Nov 25, 2024

View reviewed changes

review change - rename and accessor

91c7d87

Signed-off-by: Kavindu Dodanduwa <[email protected]>

Kavindu-Dodan force-pushed the fix/improve-object-size-metric-calculation branch from 25ea16c to 91c7d87 Compare November 25, 2024 15:56

Kavindu-Dodan requested a review from kaiyan-sheng November 25, 2024 17:10

kaiyan-sheng approved these changes Nov 25, 2024

View reviewed changes

Kavindu-Dodan merged commit 92bb2c5 into elastic:main Nov 25, 2024
22 checks passed

mergify bot mentioned this pull request Nov 25, 2024

[8.16](backport #41755) [AWS] [S3] fix: improve object size metric calculation #41777

Merged

6 tasks

mergify bot mentioned this pull request Nov 25, 2024

[8.x](backport #41755) [AWS] [S3] fix: improve object size metric calculation #41778

Merged

6 tasks

Kavindu-Dodan deleted the fix/improve-object-size-metric-calculation branch November 25, 2024 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AWS] [S3] fix: improve object size metric calculation #41755

[AWS] [S3] fix: improve object size metric calculation #41755

Kavindu-Dodan commented Nov 22, 2024 •

edited

Loading

elasticmachine commented Nov 22, 2024

andrewkroh Nov 22, 2024 •

edited

Loading

Kavindu-Dodan Nov 25, 2024

andrewkroh Nov 22, 2024 •

edited

Loading

Kavindu-Dodan Nov 25, 2024

gizas Nov 25, 2024

Kavindu-Dodan Nov 25, 2024

gizas left a comment

[AWS] [S3] fix: improve object size metric calculation #41755

[AWS] [S3] fix: improve object size metric calculation #41755

Conversation

Kavindu-Dodan commented Nov 22, 2024 • edited Loading

Proposed commit message

Checklist

How to test this PR locally

Screenshots

Footnotes

elasticmachine commented Nov 22, 2024

andrewkroh Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Kavindu-Dodan Nov 25, 2024

Choose a reason for hiding this comment

andrewkroh Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Kavindu-Dodan Nov 25, 2024

Choose a reason for hiding this comment

gizas Nov 25, 2024

Choose a reason for hiding this comment

Kavindu-Dodan Nov 25, 2024

Choose a reason for hiding this comment

gizas left a comment

Choose a reason for hiding this comment

Kavindu-Dodan commented Nov 22, 2024 •

edited

Loading

andrewkroh Nov 22, 2024 •

edited

Loading

andrewkroh Nov 22, 2024 •

edited

Loading