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

[PDOK-16462] implementation #1

Merged
merged 17 commits into from
May 23, 2024
Merged

[PDOK-16462] implementation #1

merged 17 commits into from
May 23, 2024

Conversation

roelarents
Copy link
Contributor

Omschrijving

implementation of the bulk

https://dev.kadaster.nl/jira/browse/PDOK-16462

Type verandering

  • Nieuwe feature
  • Aanpassing van de configuratie
  • Breaking change

Checklist:

  • Ik heb de code in deze PR zelf nogmaals nagekeken
  • Ik heb mijn code beter achtergelaten dan dat ik het aantrof
  • De code is leesbaar en de moeilijke onderdelen zijn voorzien van commentaar
  • Ik heb de tests toegevoegd/uitgebreid indien nodig
  • Ik heb de tests gedraaid die de werking van mijn wijziging bewijst
  • De PDOK documentatie is bijgewerkt indien nodig.
  • Er zit geen gevoelig informatie in deze PR (wachtwoorden etc)

@roelarents roelarents force-pushed the pdok-16462-implementation branch from 9eea8c4 to 7da6ea5 Compare May 22, 2024 08:29
@roelarents roelarents force-pushed the pdok-16462-implementation branch from 7e10882 to 3eda503 Compare May 22, 2024 09:38
if err != nil {
return err
}
_, err = scheduler.NewJob(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zelf actief data - op basis van scheduling - ophalen is denk ik niet optimaal. Het is sowieso niet idiomatic voor een Prometheus exporter: https://prometheus.io/docs/instrumenting/writing_exporters/#scheduling. Punt is dat je nu de interval op 2 punten moet configureren: hier in de exporter, en daarnaast nog de scrape interval in Prometheus zelf.

Idee is om het alleen in Prometheus te doen, via de scrape interval. De data kan je dan wel cachen in de exporter. Je zou prima een cache ttl van bijv 4 uur kunnen hanteren. Dan komt er nog minder load/kosten op Azure Blob.

Flow zou er dan zou uitzien:

  • 1x bij startup de cache populaten.
  • bij elke scrape uit cache serveren
  • indien ttl verstreken: cache opnieuw populaten.

Copy link
Contributor Author

@roelarents roelarents May 22, 2024

Choose a reason for hiding this comment

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

De boel verzamelen upon scrape request van Prometheus is denk ik te laat. Het verzamelen duurt namelijk minuten lang. Ik weet/denk niet dat prometheus zo lang op een scrape request response gaat wachten. De MetricsUpdater schrijft naar die prometheus.Gauge en die cachet het tot prometheus het een keer komt scrapen. De MetricsUpdater wordt weliswaar ieder uur (en bij startup meteen) door de cronjob gestart. Maar die stopt vrij snel als hij ziet dat er geen nieuw inventory report is. De load op Azure om te bepalen of er een nieuwe report is, is w.m.b. verwaarloosbaar (een container listing waar 10 tot 100 blobs in zitten.)

Edit: per saldo wordt dus maar 1 keer een inventory report doorgespit en blijft dat gecached tot het volgend inventorty report (of de container moet herstarten en de cache verdwijnt.) De cronjob om te checken of er iets nieuws is had ik op een uur gezet omdat het report dagelijks of wekelijks gemaakt wordt en een uur me dan fijnmazig genoeg leek om te checken op nieuws.

Voorbeeld van de logging:

2024/05/22 11:37:25 start updating metrics. previous run was 0001-01-01 00:00:00 +0000 UTC
2024/05/22 11:37:25 starting aggregation
2024/05/22 11:37:25 finding newest inventory run
2024/05/22 11:37:25 found newest inventory run: 2024-05-19 12:43:43 +0000 UTC
2024/05/22 11:37:25 setting up duckdb / azure blob store connection
2024/05/22 11:37:25 start querying blob inventory (might take a while)
2024/05/22 11:49:37 0 du rows processed so far
2024/05/22 11:49:37 10000 du rows processed so far
2024/05/22 11:49:37 20000 du rows processed so far
2024/05/22 11:49:37 30000 du rows processed so far
2024/05/22 11:49:37 40000 du rows processed so far
2024/05/22 11:49:37 50000 du rows processed so far
2024/05/22 11:49:37 done querying blob inventory, 51668 du rows processed
2024/05/22 11:49:37 done aggregating blob inventory, 51668 du rows processed
2024/05/22 11:49:37 start setting metrics
2024/05/22 11:49:37 (metrics count will be limited to 1000 (of 1613)
2024/05/22 11:49:37 done updating metrics for run 2024-05-19 12:43:43 +0000 UTC
2024/05/22 12:37:25 start updating metrics. previous run was 2024-05-19 12:43:43 +0000 UTC
2024/05/22 12:37:25 starting aggregation
2024/05/22 12:37:25 finding newest inventory run
2024/05/22 12:37:25 no newer blob inventory run found
2024/05/22 13:37:25 start updating metrics. previous run was 2024-05-19 12:43:43 +0000 UTC
2024/05/22 13:37:25 starting aggregation
2024/05/22 13:37:25 finding newest inventory run
2024/05/22 13:37:25 no newer blob inventory run found

Copy link
Collaborator

Choose a reason for hiding this comment

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

Je hoeft ook niet in de scope van het scrape request alle data te verzamelen. Je zou prima tijdens het scrape verzoek async de data kunnen verzamelen. Dus in een losse goroutine. Elke scrape verzoek = data uit cache serveren, en soms - daarnaast - een nieuwe inventory report read starten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, als dat de idiomatic way is ga ik dat doen. ik wou het nog vandaag doen maar ik kwam er niet aan toe. ik wil hem eerst even in k8s hangen. en dan refactor ik dit. dus ik merge hem wel vast.

example/docker-compose.yaml Show resolved Hide resolved
internal/agg/aggregator.go Outdated Show resolved Hide resolved
internal/agg/aggregator.go Show resolved Hide resolved
internal/du/azure_blob_inventory.go Outdated Show resolved Hide resolved
internal/du/azure_blob_inventory.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@roelarents roelarents force-pushed the pdok-16462-implementation branch 3 times, most recently from f29803e to 11d42c8 Compare May 22, 2024 17:42
dont ignore go.sum

PDOK-16462
PDOK-16462
@roelarents roelarents force-pushed the pdok-16462-implementation branch from 11d42c8 to dc0d00e Compare May 22, 2024 17:53
PDOK-16462
@roelarents roelarents requested a review from rkettelerij May 23, 2024 07:25
@roelarents roelarents merged commit ba13f78 into master May 23, 2024
3 checks passed
@roelarents roelarents deleted the pdok-16462-implementation branch May 23, 2024 08:21
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.

3 participants