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

feat: send bytes_received through opamp custom message #1492

Merged
merged 11 commits into from
Mar 4, 2025

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Feb 21, 2025

Which problem is this PR solving?

  • This PR enables Refinery to send bytes_received metrics through opamp messages

Short description of the changes

  • create usageTracker in opamp agent to track bytes_received metrics reported from Refinery internal metrics
  • usageTracker does all the calculation and conversion to otlp resources metrics
  • add tests

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's a overflow issue that causes a datapoint missing, I would like to be able to capture it in refinery's telemetry data. My thinking is that, that's money missing for honeycomb so I want to make sure it's visible

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fine, it's certainly not wrong. But if someone is overflowing an int64 the money missing would be at least in the millions of dollars.. I'd like to have that problem. :)

@VinozzZ VinozzZ changed the title wip feat: send bytes_received through opamp custom message Feb 25, 2025
@VinozzZ VinozzZ requested a review from codeboten February 27, 2025 17:22
@VinozzZ VinozzZ marked this pull request as ready for review February 27, 2025 17:22
@VinozzZ VinozzZ force-pushed the yingrong/refinery_opamp_bytes_received branch from d1739f5 to fca3046 Compare February 27, 2025 19:02
@VinozzZ VinozzZ changed the base branch from codeboten/opamp to yingrong/bytes_received_metrics February 27, 2025 19:02
Base automatically changed from yingrong/bytes_received_metrics to codeboten/opamp February 27, 2025 21:03
@VinozzZ VinozzZ requested a review from TylerHelmuth February 28, 2025 21:13
@VinozzZ VinozzZ force-pushed the yingrong/refinery_opamp_bytes_received branch from 84f993b to 4bbf5f0 Compare March 3, 2025 17:10
agent/agent.go Outdated
}
}

func (agent *Agent) usageReport() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is specific to sending the report and not accumulating the metrics and we update the name to reflect that?

@VinozzZ VinozzZ merged commit e329415 into codeboten/opamp Mar 4, 2025
3 checks passed
@VinozzZ VinozzZ deleted the yingrong/refinery_opamp_bytes_received branch March 4, 2025 15:03
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.

4 participants