syn2mas: Add progress reporting to log and to opentelemetry metrics #4215
Conversation
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
9228f20
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c5b41409.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://rei-syn2mas-progress-counter.matrix-authentication-service-docs.pages.dev |
reivilibre
force-pushed
the
rei/syn2mas_progress_counters
branch
from
5968aa8 to
ebad8a7
Compare
sandhose
left a comment
sandhose
left a comment
There was a problem hiding this comment.
Generally happy with the approach for the progress logs, I would just do the OTEL meters differently
crates/cli/src/commands/syn2mas.rs
Outdated
| let migrated_data_counter = METER | ||
| .u64_gauge("migrated_data") | ||
| .with_description("How many entities have been migrated so far") | ||
| .build(); | ||
| let max_data_counter = METER | ||
| .u64_gauge("max_data") | ||
| .with_description("How many entities of the given type exist (approximate)") | ||
| .build(); |
There was a problem hiding this comment.
Upping counters directly is probably performant enough? I would make those counters, static with a std::sync::LazyLock and just record every time we insert
There was a problem hiding this comment.
I didn't make them static but maybe this is OK?
crates/cli/src/commands/syn2mas.rs
Outdated
| occasional_progress_logger_task.abort(); | ||
| progress_telemetry_task.abort(); |
There was a problem hiding this comment.
Ideally this would be done through a CancellationToken, I don't particularly like aborting tasks, but since we haven't set that up in the rest of the migration process, I'm happy to keep it like this for now
crates/cli/src/commands/syn2mas.rs
Outdated
| /// Reports migration progress as OpenTelemetry metrics | ||
| async fn progress_telemetry(progress: Progress) { | ||
| let migrated_data_counter = METER | ||
| .u64_gauge("migrated_data") |
There was a problem hiding this comment.
Let's use a prefix, plus a common namespace, e.g. syn2mas.data.migrated and syn2mas.data.total. Note that when translated to Prometheus, dots will be replaced by underscores. Feel free to come up something else, this is just a suggestion
There was a problem hiding this comment.
syn2mas.entity... maybe. Don't like how vague these words are but something like 'migratee' is no better haha. I guess it doesn't exactly matter as long as it makes some sense.
…trics Add metrics directly within syn2mas, no background thread
reivilibre
force-pushed
the
rei/syn2mas_progress_counters
branch
from
6f6c20b to
9228f20
Compare
sandhose
added
A-Migration
The logging looks a bit like:
(artificially slowed down)
I did not try the metrics. The HTTP listeners aren't active so Prometheus metrics are no good, you'd have to push them to an OpenTelemetry thing and I didn't bother to do that yet.
That said, I'm not convinced by the value so perhaps it's better to just drop that aspect?
(edit maybe it'd be good to include the word 'progress' in the line there somewhere. Perhaps
target: "syn2mas::progress"or something.... not sure what's best. Happy to hear your thoughts on that)