Skip to content

Conversation

@jeffreyssmith2nd
Copy link
Contributor

Which issue does this PR close?

Closes #11754

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@jeffreyssmith2nd jeffreyssmith2nd marked this pull request as ready for review July 31, 2024 20:21
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

It seems somewhat awkward to add a "type" of metric that is logically the same bug just differs in aggregation behavior

I wonder if the same result could be achieved by writing a custom aggregation function

@jeffreyssmith2nd
Copy link
Contributor Author

It seems somewhat awkward to add a "type" of metric that is logically the same bug just differs in aggregation behavior

I don't necessarily disagree with this, but where would the custom aggregation function live/how would it be used? There are multiple places that MetricValue::aggregate is called.

iter.for_each(|metric| accum.aggregate(metric.value()));

accum.value_mut().aggregate(metric.value());

accum.value_mut().aggregate(metric.value());

@alamb
Copy link
Contributor

alamb commented Aug 5, 2024

I had some more thoughts on this PR here: #11754

@alamb alamb marked this pull request as draft August 8, 2024 15:31
@alamb
Copy link
Contributor

alamb commented Aug 8, 2024

Marking as a draft to signify this isn't waiting on another review.

It isn't clear to me how to proceed with this PR given #11754

@jeffreyssmith2nd
Copy link
Contributor Author

Marking as a draft to signify this isn't waiting on another review.

It isn't clear to me how to proceed with this PR given #11754

Thanks, I do intend to get back to #11754 but am not sure whether this PR will make sense to continue once I have time to think about it.

@github-actions
Copy link

github-actions bot commented Oct 8, 2024

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale PR has not had any activity for some time label Oct 8, 2024
@github-actions github-actions bot closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Stale PR has not had any activity for some time

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add MetricValue that returns the max during aggregation

2 participants