Skip to content

Fix accidentally swapped reporter and monitor interval#425

Merged
rockdaboot merged 2 commits intoopen-telemetry:mainfrom
zystem-io:joel/fix-time-init
Apr 3, 2025
Merged

Fix accidentally swapped reporter and monitor interval#425
rockdaboot merged 2 commits intoopen-telemetry:mainfrom
zystem-io:joel/fix-time-init

Conversation

@athre0z
Copy link
Copy Markdown
Member

@athre0z athre0z commented Apr 1, 2025

times.New signature is:

func New(reportInterval, monitorInterval,
  probabilisticInterval time.Duration) *Times

The argument order in main.go was swapped, this commit fixes that.

Default for both is 5s, but if you pass args to override the defaults, this would result in incorrect behavior.

times.New signature is:

func New(reportInterval, monitorInterval,
  probabilisticInterval time.Duration) *Times

The argument order in `main.go` was swapped, this commit fixes that.
@athre0z athre0z requested review from a team as code owners April 1, 2025 09:58
@rockdaboot
Copy link
Copy Markdown
Contributor

Wow, this shouldn't have happened.

Can you please also fix

intervals := times.New(cfg.MonitorInterval,
?

@Gandem
Copy link
Copy Markdown
Member

Gandem commented Apr 1, 2025

Since this is the second time a similar mix up happens, should we change times.New to take a struct instead as suggested in #242 (review) ? (Could be a follow-up PR - happy to draft one if needed)

@rockdaboot
Copy link
Copy Markdown
Contributor

rockdaboot commented Apr 1, 2025

Since this is the second time a similar mix up happens, should we change times.New to take a struct instead as suggested in #242 (review) ? (Could be a follow-up PR - happy to draft one if needed)

Not against it, though it implies we have do it for every function that takes 2+ args of the same type. Alternatively, we can add a test for times.New.

@rockdaboot rockdaboot merged commit d510d9c into open-telemetry:main Apr 3, 2025
26 checks passed
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.

5 participants