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

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

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Nov 25, 2024

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 1. In code level (SDK internally), this is derived using Content-Length header 2.

It seems that Cloudflare R2 (API compatible with S3) could omit Content-Length header 3. 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

image

Handling validated for plain text

image



This is an automatic backport of pull request #41755 done by [Mergify](https://mergify.com).

Footnotes

  1. https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html#API_GetObject_ResponseSyntax

  2. https://github.com/aws/aws-sdk-go-v2/blob/service/s3/v1.60.1/service/s3/deserializers.go#L5477-L5484

  3. https://community.cloudflare.com/t/no-content-length-header-when-serving-plaintext-from-r2/565521

* 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)
@mergify mergify bot requested a review from a team as a code owner November 25, 2024 19:23
@mergify mergify bot added the backport label Nov 25, 2024
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 25, 2024
@Kavindu-Dodan Kavindu-Dodan added the Team:obs-ds-hosted-services Label for the Observability Hosted Services team label Nov 25, 2024
@elasticmachine
Copy link
Collaborator

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

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 25, 2024
@Kavindu-Dodan Kavindu-Dodan enabled auto-merge (squash) November 25, 2024 19:32
@Kavindu-Dodan Kavindu-Dodan merged commit 7e92949 into 8.x Nov 25, 2024
22 checks passed
@Kavindu-Dodan Kavindu-Dodan deleted the mergify/bp/8.x/pr-41755 branch November 25, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants