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

feat: emit metrics about entity imports #847

Merged
merged 6 commits into from
Aug 12, 2024
Merged

feat: emit metrics about entity imports #847

merged 6 commits into from
Aug 12, 2024

Conversation

m90
Copy link
Contributor

@m90 m90 commented Jul 23, 2024

@m90 m90 force-pushed the feature/data-reuse-tool branch from c3d0fb1 to 32c2862 Compare July 30, 2024 05:42
@m90 m90 force-pushed the fr/import-metrics branch 2 times, most recently from 5da31a6 to 3745738 Compare July 30, 2024 07:49
Base automatically changed from feature/data-reuse-tool to main July 30, 2024 08:26
@m90 m90 force-pushed the fr/import-metrics branch 2 times, most recently from dc934a3 to d4181a6 Compare August 6, 2024 10:04

public function __construct(CollectorRegistry $registry)
{
$this->successfulCounter = $registry->getOrRegisterCounter(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a tricky decision here: if we define these counters only once, it means they will be missing from the metrics if they have never been increased yet (the controller that answers the scraper will not know about it yet as it's never been persisted).

Alternatively we could:

  • duplicate this definition in the Metrics/WikiEntityImport class
  • move all metrics definitions to the service provider and have stub metrics methods on the metrics classes themselves

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand right this isn't really a big problem? It just means that instead of 0 we will have null until the first import?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly

'host' => env('REDIS_HOST', '127.0.0.1'),
'password' => env('REDIS_PASSWORD', null),
'port' => env('REDIS_PORT', 6379),
'database' => env('REDIS_METRICS_DB', 2),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not supported by the Prometheus library that just uses a hardcoded PROMETHEUS_ prefix on keys. I shall remove the setting again.

@m90 m90 merged commit 06ff083 into main Aug 12, 2024
5 checks passed
@m90 m90 deleted the fr/import-metrics branch August 12, 2024 08:34
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.

2 participants