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

bugfix: Metrics initialization #2767

Merged
merged 6 commits into from
Dec 17, 2022

Conversation

Fabianoshz
Copy link
Contributor

@Fabianoshz Fabianoshz commented Dec 9, 2022

what

  • Adds empty metrics for prometheus when starting the server.

why

  • Prevent NaN from showing up for metrics when atlantis first starts

references

@Fabianoshz Fabianoshz requested a review from a team as a code owner December 9, 2022 22:10
@Fabianoshz Fabianoshz marked this pull request as draft December 9, 2022 22:10
@nitrocode nitrocode changed the title Metrics initilization Metrics initialization Dec 9, 2022
@Fabianoshz
Copy link
Contributor Author

Please tell me if adding the NewInstrumentedProjectCommandRunner is a good idea here, this would be the way to go in other languages but I'm not so sure if this is a good pattern when using golang. I'm also aware that the code can be simplified and more organized, just scrambled this to start from somewhere (should have explained that in the initially, sorry 😅)

@jamengual
Copy link
Contributor

Please tell me if adding the NewInstrumentedProjectCommandRunner is a good idea here, this would be the way to go in other languages but I'm not so sure if this is a good pattern when using golang. I'm also aware that the code can be simplified and more organized, just scrambled this to start from somewhere (should have explained that in the initially, sorry 😅)

hard to know about the name since in the same file there is some sort of a pattern but then the is another function that does not follow the same name pattern.... @nitrocode @lilincmu what do you guys think?

@Fabianoshz
Copy link
Contributor Author

Fabianoshz commented Dec 10, 2022

Made some changes to make this code look a little more like instrumented_pull_closed_executor file, my idea is to have something between the functions and the struct creation on the server/server.go where I would be able to initialize the metrics, in this case the NewInstrumentedProjectCommandRunner. For the other instrumented it would be their respective NewInstrumentedSomething.

I was forced to implement the function ApprovePolicies, otherwise NewApprovePoliciesCommandRunner would break on server/server.go:

atlantis/server/server.go

Lines 682 to 690 in 4d95783

approvePoliciesCommandRunner := events.NewApprovePoliciesCommandRunner(
commitStatusUpdater,
projectCommandBuilder,
instrumentedProjectCmdRunner,
pullUpdater,
dbUpdater,
userConfig.SilenceNoProjects,
userConfig.SilenceVCSStatusNoPlans,
)

It's not clear to me why this is necessary.

@Fabianoshz Fabianoshz force-pushed the origin/initialize-metrics branch 2 times, most recently from 61d5c5a to 3db6f7e Compare December 10, 2022 13:46
@Fabianoshz Fabianoshz closed this Dec 10, 2022
@Fabianoshz Fabianoshz reopened this Dec 10, 2022
@Fabianoshz
Copy link
Contributor Author

Made some mess with git here, sorry. Anyway, added the same logic for the other instrumented which I believe would be the places where this initialization would be applicable.

@nitrocode
Copy link
Member

cc @albertorm95 for an extra set of eyes

@GenPage
Copy link
Member

GenPage commented Dec 12, 2022

A function inside of the metrics package also makes sense to me. Thank you for taking the time to do this @Fabianoshz

@albertorm95
Copy link
Contributor

albertorm95 commented Dec 12, 2022

Looks good to me.

Can you share how it looks in the /metrics endpoint? (I mean the metrics with the 0 value)

@Fabianoshz
Copy link
Contributor Author

I will add the function and move this out of draft later today.

@Fabianoshz Fabianoshz marked this pull request as ready for review December 13, 2022 00:15
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.

Initialize Atlantis metrics with zeroes
5 participants