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

fix: prevent potential misconfiguration of metricsInterval (#284) #638

Conversation

ritwikranjan
Copy link
Contributor

@ritwikranjan ritwikranjan commented Aug 23, 2024

Description

I have added a new field metricsIntervalDuration of type time.Duration. It will super cede the metricsInterval configuration of the same type. The older key is not removed to keep backward compatibility. The change is done to prevent potential misconfiguration that can happen by assigning a duration to metricsInterval when it is converted to seconds.

Related Issue

Issue #284

Checklist

  • I have read the contributing documentation.
  • I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
  • I have correctly attributed the author(s) of the code.
  • I have tested the changes locally.
  • I have followed the project's style guidelines.
  • I have updated the documentation, if necessary.
  • I have added tests, if applicable.

Screenshots (if applicable) or Testing Completed

The logs showing that retina agent runs successffully after making the changes.
retina-after-chnage

This shows the log line that metricsInterval should not be used.
retina-using-metrics-interval

Additional Notes

I was not able to us the zap logger as it is probably not initialized when the GetConfig is called or I might be missing how to use it so i used the go log library. Do let me know if we need to change that.


Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

@ritwikranjan ritwikranjan requested a review from a team as a code owner August 23, 2024 10:32
@ritwikranjan ritwikranjan force-pushed the 284-prevent-the-potential-for-misconfiguration-of-metricsinterval branch from 19d5248 to 9f74609 Compare August 23, 2024 13:33
@nddq nddq added type/fix Fixes something area/metrics labels Aug 23, 2024
@nddq nddq linked an issue Aug 23, 2024 that may be closed by this pull request
pkg/config/config.go Outdated Show resolved Hide resolved
@ritwikranjan ritwikranjan requested review from a team and removed request for xiaozhiche320 and karina-ranadive August 26, 2024 23:17
timraymond
timraymond previously approved these changes Aug 27, 2024
@ritwikranjan ritwikranjan force-pushed the 284-prevent-the-potential-for-misconfiguration-of-metricsinterval branch from 58ce584 to 57fb848 Compare August 28, 2024 10:50
@ritwikranjan ritwikranjan force-pushed the 284-prevent-the-potential-for-misconfiguration-of-metricsinterval branch from 57fb848 to 62ec411 Compare August 28, 2024 11:00
@ibezrukavyi ibezrukavyi self-requested a review August 28, 2024 12:47
Copy link

@ibezrukavyi ibezrukavyi left a comment

Choose a reason for hiding this comment

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

Re-approve merge from the main branch

Merged via the queue into microsoft:main with commit 6ff0ad2 Aug 29, 2024
22 checks passed
@ritwikranjan ritwikranjan deleted the 284-prevent-the-potential-for-misconfiguration-of-metricsinterval branch August 29, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prevent the potential for misconfiguration of MetricsInterval
5 participants