Skip to content

[telemetrygen] Add continuous mode #40225

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Aug 13, 2025
Merged

[telemetrygen] Add continuous mode #40225

merged 10 commits into from
Aug 13, 2025

Conversation

araiu
Copy link
Contributor

@araiu araiu commented May 22, 2025

Add continuous mode to telemetrygen cmd.
This can be achieved by setting a the --duration to a very large value, but --continuous is explicit.

@araiu araiu requested review from a team, codeboten and mx-psi as code owners May 22, 2025 11:45
@github-actions github-actions bot added the cmd/telemetrygen telemetrygen command label May 22, 2025
@github-actions github-actions bot requested a review from Erog38 May 22, 2025 11:46
@atoulme
Copy link
Contributor

atoulme commented May 22, 2025

Please add a changelog - how about we change the logic, and if no --duration is provided, we just run continuously?

@atoulme atoulme marked this pull request as draft May 22, 2025 12:10
@atoulme
Copy link
Contributor

atoulme commented May 22, 2025

Moving to draft while you address feedback and CI. Please mark ready for review when done.

@araiu
Copy link
Contributor Author

araiu commented May 22, 2025

Please add a changelog - how about we change the logic, and if no --duration is provided, we just run continuously?

Added a changelog as well. Thank you for the heads up.

The default logic is to send a single signal and then exit, which we use pretty often, but we would want to have this run continuously for health checks.

So by default (no flag set) telemetrygen will send a signal and exit, if --duration is set, it will exit after the configured time, and with --continuous, it will run continuously. I agree to the current logic, but I am open to suggestions.

@araiu araiu force-pushed the telemetrygen/add_continuous_mode branch from 74a9845 to eb59e0e Compare May 22, 2025 12:38
@atoulme
Copy link
Contributor

atoulme commented May 22, 2025

Thanks for explaining - ok in that case the additional flag makes sense.

@mx-psi
Copy link
Member

mx-psi commented May 23, 2025

What about doing --duration inf or some other kind of special value? (I am not sure which alternative is best but I think it is worth discussing)

codeboten
codeboten reviewed May 23, 2025
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

i like the idea of using a duration value to avoid having to check one more flag, but i don't feel strongly about this

@araiu araiu force-pushed the telemetrygen/add_continuous_mode branch from 10d4db3 to d2b4c86 Compare May 23, 2025 15:14
@araiu
Copy link
Contributor Author

araiu commented May 23, 2025

I like the idea of an inf value for duration, but wouldn't it be confusing for a flag to accept a duration or an additional specific string ?

@mx-psi
Copy link
Member

mx-psi commented May 26, 2025

I like the idea of an inf value for duration, but wouldn't it be confusing for a flag to accept a duration or an additional specific string ?

inf is already the way infinity is represented as a string when it comes to floats (see strconv.ParseFloat), so I think it's not that confusing

@araiu araiu force-pushed the telemetrygen/add_continuous_mode branch from ebba9c7 to eef1bf4 Compare May 26, 2025 19:35
@araiu
Copy link
Contributor Author

araiu commented May 26, 2025

I like the idea of an inf value for duration, but wouldn't it be confusing for a flag to accept a duration or an additional specific string ?

inf is already the way infinity is represented as a string when it comes to floats (see strconv.ParseFloat), so I think it's not that confusing

Agree, can't argue with this 😅 . Removed the continuous flag and worked around allowing inf as duration

@github-actions github-actions bot mentioned this pull request May 27, 2025
Closed
@github-actions github-actions bot mentioned this pull request Jun 3, 2025
Closed
@github-actions github-actions bot mentioned this pull request Jun 10, 2025
Closed
@araiu araiu marked this pull request as ready for review June 12, 2025 10:49
@araiu araiu requested a review from codeboten June 12, 2025 10:49
@github-actions github-actions bot mentioned this pull request Jun 17, 2025
Closed
bogdan-st
bogdan-st approved these changes Jun 23, 2025
View reviewed changes
Copy link
Contributor

@bogdan-st bogdan-st left a comment

Choose a reason for hiding this comment

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

Would love this option!

@github-actions github-actions bot mentioned this pull request Jun 24, 2025
Closed
Erog38
Erog38 suggested changes Jun 27, 2025
View reviewed changes
cmd/telemetrygen/internal/common/config.go Outdated
}

// DurationWithInf is a custom type that can handle both regular durations and "inf" value
type DurationWithInf time.Duration
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, placing this here will break programmatic usage of telemetrygen since it's an internal package. I'd be inclined to put this in pkg/types or a similarly named package in order to keep Duration as something which can be set by either the CLI or an external package.

@atoulme's original idea of setting to continuous if no duration is set made the most sense to me as it's consistent with our rate configuration behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the type out of internal and update the defaults to run continuously, with a rate of 1 signal.

This was referenced Jul 1, 2025
Closed
Closed
@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 12, 2025
@github-actions github-actions bot mentioned this pull request Jul 15, 2025
Closed
@mx-psi
Copy link
Member

mx-psi commented Jul 21, 2025

Apologies for the radio silence from my side. I agree with @Erog38 on the change. I would like to take this change in, so I may fix this myself if you don't have the time @araiu

@github-actions github-actions bot removed the Stale label Jul 22, 2025
@araiu
Copy link
Contributor Author

araiu commented Jul 22, 2025

Apologies for the radio silence from my side. I agree with @Erog38 on the change. I would like to take this change in, so I may fix this myself if you don't have the time @araiu

No worries, I won't have time for the next two weeks so feel free add it. Thank you! 🙇

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 6, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 6, 2025
@atoulme atoulme removed the Stale label Aug 6, 2025
@atoulme
Copy link
Contributor

atoulme commented Aug 6, 2025

Removing stale - @araiu it's been 2 weeks, would you like to try and continue the work here?

@araiu
Copy link
Contributor Author

araiu commented Aug 7, 2025

Removing stale - @araiu it's been 2 weeks, would you like to try and continue the work here?

@atoulme, yes. I'll take another swing at this. Thank you

@mx-psi
Copy link
Member

mx-psi commented Aug 7, 2025

Forgot to state the obvious but I didn't find the time in these two weeks to work on this 😅

@araiu araiu force-pushed the telemetrygen/add_continuous_mode branch from 1d580a5 to b60e6de Compare August 11, 2025 14:41
@araiu araiu requested a review from Erog38 August 11, 2025 15:06
@araiu
Copy link
Contributor Author

araiu commented Aug 11, 2025

Forgot to state the obvious but I didn't find the time in these two weeks to work on this 😅

No worries. Glad I got some time to look more into it

mx-psi
mx-psi approved these changes Aug 11, 2025
View reviewed changes
@mx-psi mx-psi merged commit 60a7ffc into open-telemetry:main Aug 13, 2025
178 of 185 checks passed
@bogdan-st bogdan-st mentioned this pull request Aug 25, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mx-psi mx-psi mx-psi approved these changes

@codeboten codeboten Awaiting requested review from codeboten codeboten is a code owner

@Erog38 Erog38 Awaiting requested review from Erog38

+1 more reviewer

@bogdan-st bogdan-st bogdan-st approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mx-psi mx-psi

@songy23 songy23

Labels

cmd/telemetrygen telemetrygen command waiting-for-code-owners

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@araiu @atoulme @mx-psi @codeboten @Erog38 @bogdan-st @songy23