fixbug: Update commandName in RunAndEmitStats #3437
Conversation
| @@ -35,30 +35,31 @@ func NewInstrumentedProjectCommandRunner(scope tally.Scope, projectCommandRunner | |||
| } | |||
|
|
|||
| func (p *InstrumentedProjectCommandRunner) Plan(ctx command.ProjectContext) command.ProjectResult { | |||
| return RunAndEmitStats("plan", ctx, p.projectCommandRunner.Plan, p.scope) | |||
| return RunAndEmitStats(ctx, p.projectCommandRunner.Plan, p.scope) | |||
There was a problem hiding this comment.
Any chance of adding a unit or e2e test here?
There was a problem hiding this comment.
I didn't see this comment, let me check
There was a problem hiding this comment.
I don't know how to develop the test 🥺, is there a boilerplate or something I can use as guide, should I just look at other tests?
There was a problem hiding this comment.
For now @albertorm95 , please look to other tests for examples.
Then we can create server/events/instrumented_project_command_runner_test.go and then write a test that will test the output of the RunAndEmitStats function.
There was a problem hiding this comment.
@nitrocode I try to develop the code, but it looks like there is a lot of work to do in order to make that NewInstrumentedProjectCommandRunner or RunAndEmitStats testable, looks like I need to develop a mock? and then add it to the events.NewPlanCommandRunner() and same for apply, etc.., I don't feel confident enough to do this
The latest release is already failing because of this. What do you suggest to do in this case?
There was a problem hiding this comment.
I'm tempted to merge based on the above comments regarding testing and how you tested end to end.
cc: @GenPage @jamengual thoughts?
GenPage
added
waiting-on-response
|
This should fix #3439 |
|
@pseudomorph Can you test on your end before merging? |
|
@GenPage - I don't have Prometheus set up. Perhaps @chriskuchin would be able to help test? Is there an image published for this change? |
|
My only running atlantis instance is the one my team uses. I might be able to roll a test image but if someone has a local setup the prometheus config would just require in the server side yaml. You won't actually need a prometheus server for it to trigger. metrics:
prometheus:
endpoint: "/metrics" |
|
It's either we merge this or revert #3416. I'm OK with reverting and also OK with merging this. However, we could merge this and it results in another bad build whereas reverting will at least ensure a healthy build. |
|
Ok let's merge and hope for the best. If not, we can revert both prs |
|
A new image will be created after this is finished in 20 to 30 mins https://github.com/runatlantis/atlantis/actions/runs/5063551272/jobs/9090284221 @albertorm95 (et al) once the job finishes, the following images will be available. Please give one a try in your setup and confirm it works. Then we can release 0.24.1. |
done |
what
CommandName.String()from thectxinstead of fixed string.why
commandNameargument fromRunAndEmitStatsfunction you can see it clearly in the first commit it was only use forlogger.Err()commandNameargument in order to fix the previous mention issue and that when we saw the first panic that was cause bypolicy checkstring without_: https://github.com/runatlantis/atlantis/pull/3416/files#diff-161796dd18c10dc19d3513064a1bcc27d4a5e2fc9c2276d82e741aa85f89c79eR63{ "level": "error", "ts": "2023-05-23T10:35:45.772Z", "caller": "events/command_runner.go:429", "msg": "PANIC: descriptor Desc{fqName: \"atlantis_project_policy check_execution_time\", help: \"atlantis_project_policy check_execution_time summary\", constLabels: {}, variableLabels: [terraform_version workspace pr_number base_repo project project_path]} is invalid: \"atlantis_project_policy check_execution_time\" is not a valid metric name\ngithub.com/uber-go/[email protected]+incompatible/prometheus/reporter.go:306 (0xb02707)\ngithub.com/uber-go/[email protected]+incompatible/prometheus/reporter.go:554 (0xb02062)\ngithub.com/uber-go/[email protected]+incompatible/scope.go:363 (0x9e5787)\ngithub.com/runatlantis/atlantis/server/events/instrumented_project_command_runner.go:66 (0xfa8e82)\ngithub.com/runatlantis/atlantis/server/events/instrumented_project_command_runner.go:42 (0xfa86b4)\ngithub.com/runatlantis/atlantis/server/events/project_command_pool_executor.go:48 (0xfbf121)\ngithub.com/runatlantis/atlantis/server/events/policy_check_command_runner.go:65 (0xfb2107)\ngithub.com/runatlantis/atlantis/server/events/plan_command_runner.go:165 (0xfaf930)\ngithub.com/runatlantis/atlantis/server/events/plan_command_runner.go:288 (0xfb1304)\ngithub.com/runatlantis/atlantis/server/events/command_runner.go:174 (0xf94f8d)\nruntime/asm_amd64.s:1598 (0x46b2e0)\n", "json": { "repo": "foo", "pull": "1234" }, "stacktrace": "github.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).logPanics\n\tgithub.com/runatlantis/atlantis/server/events/command_runner.go:429\nruntime.gopanic\n\truntime/panic.go:884\ngithub.com/uber-go/tally/prometheus.NewReporter.func1\n\tgithub.com/uber-go/[email protected]+incompatible/prometheus/reporter.go:306\ngithub.com/uber-go/tally/prometheus.(*reporter).AllocateTimer\n\tgithub.com/uber-go/[email protected]+incompatible/prometheus/reporter.go:554\ngithub.com/uber-go/tally.(*scope).Timer\n\tgithub.com/uber-go/[email protected]+incompatible/scope.go:363\ngithub.com/runatlantis/atlantis/server/events.RunAndEmitStats\n\tgithub.com/runatlantis/atlantis/server/events/instrumented_project_command_runner.go:66\ngithub.com/runatlantis/atlantis/server/events.(*InstrumentedProjectCommandRunner).PolicyCheck\n\tgithub.com/runatlantis/atlantis/server/events/instrumented_project_command_runner.go:42\ngithub.com/runatlantis/atlantis/server/events.runProjectCmds\n\tgithub.com/runatlantis/atlantis/server/events/project_command_pool_executor.go:48\ngithub.com/runatlantis/atlantis/server/events.(*PolicyCheckCommandRunner).Run\n\tgithub.com/runatlantis/atlantis/server/events/policy_check_command_runner.go:65\ngithub.com/runatlantis/atlantis/server/events.(*PlanCommandRunner).runAutoplan\n\tgithub.com/runatlantis/atlantis/server/events/plan_command_runner.go:165\ngithub.com/runatlantis/atlantis/server/events.(*PlanCommandRunner).Run\n\tgithub.com/runatlantis/atlantis/server/events/plan_command_runner.go:288\ngithub.com/runatlantis/atlantis/server/events.(*DefaultCommandRunner).RunAutoplanCommand\n\tgithub.com/runatlantis/atlantis/server/events/command_runner.go:174" }ctx.CommandName.String()value that already have the correct command names,atlantis/server/events/command/name.go
Lines 59 to 79 in 8e72ab5
tests
Debugging:
Before:
policy_check:
and then panic:
After:
plan:
policy_check
approve_policies
apply
state or state rm
Metrics:
Show Metrics
references