Skip to content

Conversation

@jackshirazi
Copy link
Contributor

Description:

setInterval() was void, but it's useful when resetting the interval to know what the previous interval was. We could add a new getInterval, but it seems more efficient to just return it from the setInterval()

Existing Issue(s):

Testing:

Documentation:

Outstanding items:

Note this technically breaks the existing InferredSpans class API, but shouldn't impact anything because existing calls to setInterval() are not assigning a return value, so will continue to work fine

Alternative Considered

We could instead add a getInterval() call

@jackshirazi jackshirazi marked this pull request as ready for review October 13, 2025 13:18
@jackshirazi jackshirazi requested a review from a team as a code owner October 13, 2025 13:18
@Nullable
public static Duration setProfilerInterval(Duration interval) {
InferredSpansProcessor p = instance;
if (p != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I prefer this flipped, but not a huge deal.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

LGTM

@laurit
Copy link
Contributor

laurit commented Oct 14, 2025

Note this technically breaks the existing InferredSpans class API, but shouldn't impact anything because existing calls to setInterval() are not assigning a return value, so will continue to work fine

This would only be true when the caller is recompiled against the new api, if is is not it will fail with NoSuchMethodError. I guess it is fine since probably nobody besides you uses it.

@breedx-splk breedx-splk added this pull request to the merge queue Oct 14, 2025
Merged via the queue into open-telemetry:main with commit 7fc1fa8 Oct 14, 2025
25 checks passed
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