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

wip #1492

Draft
wants to merge 1 commit into
base: codeboten/opamp
Choose a base branch
from
Draft

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Feb 21, 2025

Which problem is this PR solving?

Short description of the changes

@VinozzZ VinozzZ requested a review from a team as a code owner February 21, 2025 23:54
@VinozzZ VinozzZ marked this pull request as draft February 21, 2025 23:54
@VinozzZ VinozzZ force-pushed the yingrong/refinery_opamp_bytes_received branch from 62b19f4 to b7bdf3f Compare February 21, 2025 23:58
return 0, fmt.Errorf("value %f is too large to convert to int64", value)
}
if value < math.MinInt64 {
return 0, fmt.Errorf("value %f is too small to convert to int64", value)
Copy link
Contributor

@kentquirk kentquirk Feb 22, 2025

Choose a reason for hiding this comment

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

"too small" is probably the wrong phrase here -- MinInt64 is a very large negative number.

I'm also wondering why you bothered to introduce this function at all. It's not actually wrong, but if you had just used int64(value) it would only error if a single report is more than about 9000 terabytes. Then addOTLPSum() wouldn't have to return errors either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants