Conversation
michaeljguarino
left a comment
michaeljguarino
left a comment
There was a problem hiding this comment.
this would need graphql exposure of the deployment_settings metrics fields, and also integration into the k8s operator too. You can probably get cursor agent to do it for you tbh.
| @@ -0,0 +1,95 @@ | |||
| defmodule Console.Otel.Exporter do | |||
There was a problem hiding this comment.
Is there no library that handles this at the moment? There might not be, but would be nice to use if so
There was a problem hiding this comment.
I think existing library opentelemetry_exporter is still dev status (not guaranteed to be stable)?
| """ | ||
| @spec maybe_export_metrics() :: :ok | {:error, term} | ||
| def maybe_export_metrics do | ||
| case Settings.fetch() do |
There was a problem hiding this comment.
convert this into a with expression (almost all nested cases can become them)
lib/console/otel/cron.ex
Outdated
| Use this for direct invocation (testing, manual runs). | ||
| """ | ||
| @spec export_metrics() :: :ok | {:error, term} | ||
| def export_metrics do |
lib/console/otel/cron.ex
Outdated
| end | ||
| end | ||
|
|
||
| defp should_run?(crontab) do |
There was a problem hiding this comment.
i don't think this will work w/o some statefulness (you need to keep around the last run date somewhere to compute the next run time, we do that everywhere in the db-bound crons).
A way to simulate that would be to convert this into a genserver, keep the last run time in genserver state, and only run the metrics export if the node is a determined leader (use the Console.ClusterRing module to determine that)
lib/console/otel/metrics_exporter.ex
Outdated
| [] | ||
| |> Stream.concat(build_service_metrics(timestamp)) | ||
| |> Stream.concat(build_cluster_metrics(timestamp)) | ||
| |> Enum.to_list() |
There was a problem hiding this comment.
doing an enum.to_list eliminates the whole purpose of the stream (maintaining O(1) memory usage). What you should do here is instead pipe the concat'ed stream into Stream.chunk_every(100 or some other chunk size, and then pipe that to the exporter.
lib/console/otel/metrics_exporter.ex
Outdated
| |> Stream.flat_map(&build_cluster_metrics_list(&1, timestamp)) | ||
| end | ||
|
|
||
| defp build_service_metric(%Service{} = service, timestamp) do |
There was a problem hiding this comment.
mostly a style nit, but i'd consider moving this metric formatting code to another module
lib/console/otel/metrics_exporter.ex
Outdated
| |> DateTime.to_naive() | ||
| |> NaiveDateTime.add(60, :second) | ||
|
|
||
| next = Crontab.Scheduler.get_next_run_date!(expr, base_time) |
There was a problem hiding this comment.
i'd generally make these a with expression with {:ok, _} tuple matches down the line so you don't needlessly crash the genserver on bad input
michaeljguarino
left a comment
michaeljguarino
left a comment
There was a problem hiding this comment.
looks mostly fine, minus a few nits
| defp do_export(endpoint) do | ||
| timestamp = DateTime.utc_now() | ||
|
|
||
| Repo.transaction(fn -> |
There was a problem hiding this comment.
don't worry about the transaction here, it's read-only so no need to roll back
lib/console/otel/metrics_exporter.ex
Outdated
| count | ||
| end | ||
| end) | ||
| |> case do |
There was a problem hiding this comment.
can probably just make this |> then(&Logger.info("Exported #{&1} metrics to #{enpoint}"). It's not hard to no 0 metrics implies it didn't export
There was a problem hiding this comment.
one thing that still isn't here is the kubernetes CRD definition for setting these fields. You need to:
- Expose the inputs in gql so the client can modify them
- Modify the crd type
- Modify the controller to wire it up
Oftentimes i can just get ai to do this for me now, but you probably need to familiarize yourself enough with the structure to prompt it well
kinjalh
force-pushed
the
otel-exports
branch
2 times, most recently
from
b4072cd to
85dfc18
Compare
2 participants
Add OTel exports
Test Plan
Unit tests
Checklist
Plural Flow: console