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

[processor/deltatocumulative]: timer-based expiry #31625

Merged
merged 2 commits into from Mar 6, 2024

Conversation

sh0rez
Copy link
Contributor

@sh0rez sh0rez commented Mar 6, 2024

Description: Moves from complex preemptive expiry to a plain 1 minute timer

Link to tracking Issue: #31615 (comment)

Resolves #31615

@sh0rez sh0rez requested a review from a team as a code owner March 6, 2024 14:00
@sh0rez sh0rez requested a review from evan-bradley March 6, 2024 14:00
@sh0rez
Copy link
Contributor Author

sh0rez commented Mar 6, 2024

@RichieSams please take a look :)

aggr streams.Aggregator[data.Number]
exp *streams.Expiry[data.Number]
aggr streams.Aggregator[data.Number]
stale *staleness.Staleness[data.Number]
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: You could name this say, nums in preparation for having multiple. (IE for histograms, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's do that when its needed. I'd like to experiment with the type system more if we can't just drop the datapoint narrowing on the aggregator altogether

@RichieSams
Copy link
Contributor

Added a minor naming comment. Otherwise LGTM :)

@dmitryax dmitryax added the Run Windows Enable running windows test on a PR label Mar 6, 2024
@crobert-1
Copy link
Member

Minor nit: @sh0rez can you update the issue description to say "Resolves #31615"? This will allow our GitHub automation to automatically close the issue (reducing the chance of us forgetting) 👍

@dmitryax dmitryax merged commit 9929562 into open-telemetry:main Mar 6, 2024
155 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 6, 2024
@dmitryax
Copy link
Member

dmitryax commented Mar 6, 2024

Thank you, @sh0rez!

jpkrohling pushed a commit that referenced this pull request Mar 11, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

#31488
was recently merged and has broken `build-and-test` for all builds due
to a lint failure. It looks like two PRs were worked on in parallel, the
aforementioned one, as well as
#31625.
#31625 renamed `exp` to `stale`, but the most recently merged PR was
referencing the original `exp` variable.

This is an unreleased component, and is simply fixing a bug, so I don't
think this should have a changelog.
DougManton pushed a commit to DougManton/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:** Moves from complex preemptive expiry to a plain 1
minute timer

**Link to tracking Issue:**
open-telemetry#31615 (comment)

Resolves
open-telemetry#31615
DougManton pushed a commit to DougManton/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

open-telemetry#31488
was recently merged and has broken `build-and-test` for all builds due
to a lint failure. It looks like two PRs were worked on in parallel, the
aforementioned one, as well as
open-telemetry#31625.
open-telemetry#31625 renamed `exp` to `stale`, but the most recently merged PR was
referencing the original `exp` variable.

This is an unreleased component, and is simply fixing a bug, so I don't
think this should have a changelog.
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:** Moves from complex preemptive expiry to a plain 1
minute timer

**Link to tracking Issue:**
open-telemetry#31615 (comment)

Resolves
open-telemetry#31615
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

open-telemetry#31488
was recently merged and has broken `build-and-test` for all builds due
to a lint failure. It looks like two PRs were worked on in parallel, the
aforementioned one, as well as
open-telemetry#31625.
open-telemetry#31625 renamed `exp` to `stale`, but the most recently merged PR was
referencing the original `exp` variable.

This is an unreleased component, and is simply fixing a bug, so I don't
think this should have a changelog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/deltatocumulative Run Windows Enable running windows test on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[processor/deltatocumulative] internal/streams.TestExpiry fails on Windows
5 participants