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

[receiver/awsfirehose] remove error logging on gzip.Reader type assertion failure due to nil value #38352

Merged
merged 2 commits into from
Mar 5, 2025

Conversation

yiquanzhou
Copy link
Contributor

Description

Fixing a bug.

We see a lot of error logs after upgrading opentelemetry-collector-contrib to 0.120.0:

Expected *gzip.Reader, got *gzip.Reader

The root cause is that the type assertion in the following code could return ok=false when r is nil. This could happen when the marshaller is instantiated and there's no value in gzipPool yet.

r, ok := u.gzipPool.Get().(*gzip.Reader)
if !ok {
  u.logger.Error(fmt.Sprintf("Expected *gzip.Reader, got %T", r))
  // Fall through and create a new *gzip.Reader (r == nil)
}

This PR removes the error log. Since we are instantiating the reader only in the line right below

r, err = gzip.NewReader(bytes.NewReader(compressedRecord))

I don't think checking the type assertion result and logging an error is really necessary.

@yiquanzhou yiquanzhou requested a review from a team as a code owner March 4, 2025 13:35
@yiquanzhou yiquanzhou requested a review from TylerHelmuth March 4, 2025 13:35
@github-actions github-actions bot requested a review from Aneurysm9 March 4, 2025 13:35
@yiquanzhou yiquanzhou changed the title [receiver/awsfirehose] remove error logging on gzip.Reader type assertion failure [receiver/awsfirehose][chore] remove error logging on gzip.Reader type assertion failure Mar 4, 2025
@atoulme
Copy link
Contributor

atoulme commented Mar 4, 2025

Please add a changelog

@yiquanzhou yiquanzhou changed the title [receiver/awsfirehose][chore] remove error logging on gzip.Reader type assertion failure [receiver/awsfirehose] remove error logging on gzip.Reader type assertion failure Mar 4, 2025
@yiquanzhou yiquanzhou changed the title [receiver/awsfirehose] remove error logging on gzip.Reader type assertion failure [receiver/awsfirehose] remove error logging on gzip.Reader type assertion failure due to nil value Mar 4, 2025
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM

@atoulme atoulme merged commit 2e975fe into open-telemetry:main Mar 5, 2025
156 of 158 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 5, 2025
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.

3 participants