perf(milestones): refactor

[muhlemmer]

Which Problems Are Solved

Milestones used existing events from a number of aggregates. OIDC session is one of them. We noticed in load-tests that the reduction of the oidc_session.added event into the milestone projection is a costly business with payload based conditionals. A milestone is reached once, but even then we remain subscribed to the OIDC events. This requires the projections.current_states to be updated continuously.

How the Problems Are Solved

The milestone creation is refactored to use dedicated events instead. The command side decides when a milestone is reached and creates the reached event once for each milestone when required.

Additional Changes

In order to prevent reached milestones being created twice, a migration script is provided. When the old projections.milestones table exist, the state is read from there and v2 milestone aggregate events are created, with the original reached and pushed dates.

Additional Context

• Closes https://github.com/zitadel/zitadel/issues/8800

[github-actions[bot]]

Thanks for your contribution @muhlemmer! 🎉

Please make sure you tick the following checkboxes before marking this Pull Request (PR) as ready for review:

• [x] I am happy with the code
• [x] Documentations and examples are up-to-date
• [x] Logical behavior changes are tested automatically
• [x] No debug or dead code
• [x] My code has no repetitions
• [x] The PR title adheres to the conventional commit format
• [x] The example texts in the PR description are replaced.
• [x] If there are any open TODOs or follow-ups, they are described in issues and link to this PR
• [x] If there are deviations from a user stories acceptance criteria or design, they are agreed upon with the PO and documented.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 28, 2024 8:26am

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 69.18239% with 147 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@54f1c0b). Learn more about missing BASE report. Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/setup/36.go	37.68%	40 Missing and 3 partials :warning:
internal/repository/milestone/type_enumer.go	30.23%	28 Missing and 2 partials :warning:
internal/command/cache.go	54.34%	15 Missing and 6 partials :warning:
internal/command/oidc_session.go	50.00%	8 Missing and 4 partials :warning:
internal/command/project.go	55.00%	6 Missing and 3 partials :warning:
...rnal/eventstore/repository/mock/repository.mock.go	0.00%	8 Missing :warning:
internal/query/milestone.go	33.33%	4 Missing :warning:
internal/api/authz/instance.go	57.14%	2 Missing and 1 partial :warning:
internal/command/command.go	25.00%	2 Missing and 1 partial :warning:
internal/command/project_application_oidc.go	40.00%	2 Missing and 1 partial :warning:
... and 6 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8788   +/-   ##
=======================================
  Coverage        ?   62.59%           
=======================================
  Files           ?     1518           
  Lines           ?   143249           
  Branches        ?        0           
=======================================
  Hits            ?    89671           
  Misses          ?    49223           
  Partials        ?     4355           

Flag	Coverage Δ
core-integration-tests-postgres	36.15% <63.24%> (?)
core-unit-tests	46.80% <61.59%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[muhlemmer]

CC @eliobischof: when this is released the milestone projection will look like:

CREATE TABLE IF NOT EXISTS projections.milestones2
(
    instance_id text COLLATE pg_catalog."default" NOT NULL,
    type smallint NOT NULL,
    reached_date timestamp with time zone,
    last_pushed_date timestamp with time zone,
    CONSTRAINT milestones2_pkey PRIMARY KEY (instance_id, type)
)

[eliobischof]

CC @eliobischof: when this is released the milestone projection will look like:

CREATE TABLE IF NOT EXISTS projections.milestones2
(
    instance_id text COLLATE pg_catalog."default" NOT NULL,
    type smallint NOT NULL,
    reached_date timestamp with time zone,
    last_pushed_date timestamp with time zone,
    CONSTRAINT milestones2_pkey PRIMARY KEY (instance_id, type)
)

Thanks for the information 👍

[muhlemmer]

I would like to discuss the newly added second transactions in the command package, this could lead to inconsistent states and return "wrong" errors to the user. for example a instance can be removed but the user gets an error because the transaction adding the milestone failed

I though you explained me not too long ago separate aggregates should be pushed in separate transactions. If that's not the case, I will change the code to push once.

[adlerhurst]

I would like to discuss the newly added second transactions in the command package, this could lead to inconsistent states and return "wrong" errors to the user. for example a instance can be removed but the user gets an error because the transaction adding the milestone failed

I though you explained me not too long ago separate aggregates should be pushed in separate transactions. If that's not the case, I will change the code to push once.

I agree but my fear is that the second push fails and the user gets an error eventhough the command was successful

[muhlemmer]

@adlerhurst, changed the code to use a single push including any reached milestones. Please re-review.

[muhlemmer]

Added an error check. Can you please re-approve @adlerhurst ?