-
Notifications
You must be signed in to change notification settings - Fork 61
Add telemetry metrics for sandbox usage #4342
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
base: main
Are you sure you want to change the base?
Conversation
Add PromEx plugin to track sandbox-related metrics exposed to Prometheus: - lightning.sandbox.created.count - sandbox provisioned - lightning.sandbox.merged.count - sandbox merged into target - lightning.sandbox.deleted.count - sandbox manually deleted - lightning.workflow.saved.count (is_sandbox tag) - workflow saves by project type - lightning.provisioner.import.count (is_sandbox tag) - imports by project type
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4342 +/- ##
==========================================
+ Coverage 89.21% 89.30% +0.09%
==========================================
Files 425 426 +1
Lines 20011 20024 +13
==========================================
+ Hits 17852 17882 +30
+ Misses 2159 2142 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rorymckinley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartc Nice! Always excited to see metrics being added.
I disagree with the assertion that not seeding the event is the lesser of the two evils, as I think the value add is very low compared to seeding - but I have already presented my best argument in this regard, so not going to rehash it further.
Other than that, some small niggles about a couple of metrics that feel like they should belong somewhere else - but not serious enough to block merging if you disagree.
| |> get_assoc(:workflows) | ||
| |> Enum.each(&Workflows.publish_kafka_trigger_events/1) | ||
|
|
||
| Lightning.Projects.SandboxPromExPlugin.fire_provisioner_import_event( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartc It feels a bit weird for the SandboxPromExPlugin to be responsible for capturing provisioner import events that aren't related to sandboxes? It may be better to do one of the following:
- Only fire the provisioner_import_event if the calling code knows that the updated_project is a sandbox
- Have a ProvisionerPromExPlugin that fires an import event and differentiates between sandbox and non-sandbox.
| Lightning.Repo.get(Lightning.Projects.Project, workflow.project_id) | ||
| |> Lightning.Projects.Project.sandbox?() | ||
|
|
||
| Lightning.Projects.SandboxPromExPlugin.fire_workflow_saved_event( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartc same spidey-sense tingle as for fire_provisioner_import_event.
| } | ||
| end | ||
|
|
||
| test "delete_sandbox does not emit telemetry event on unauthorized" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartc I am guessing there is no robust way to trigger a deletion failure, to ensure that does not emit an event?
Description
This PR adds Prometheus telemetry metrics to track sandbox project usage. The new
SandboxPromExPluginmodule emits counters for:Closes #4101
Validation steps
mix test- all tests pass including new telemetry testsAdditional notes for the reviewer
ObanManagerTestsynchronous to fix a flaky test unrelated to this featureAI Usage
Pre-submission checklist