added validation checks for the werft API #9064

fullmetalrooster · 2022-04-01T11:28:18Z

Description

This adds some validation test to the werft.ts API.

It checks if a phase is already present with the same name.
A list is added that contains information about the phases and their slices.
It checks if a slice has already been closed when a log entry is created with the same name.
It checks if all slices inside a phase has been closed when the phase is closed.

UPDATE (2022-04-06): This is also fixing #9100 by ending phases explicitly. I made it optional adding the name of the phase to end it.

Related Issue(s)

Fixes #8707 #9100

How to test

You can test the changes by violating a check that has been implemented by this commit.
Here are some examples:
This commit does not have any problems: https://werft.gitpod-dev.com/job/gitpod-build-wth-test.174/raw
This commit does not close all slices: https://werft.gitpod-dev.com/job/gitpod-build-wth-test.175/raw
This commit adds a log entry when the slice has already been closed: https://werft.gitpod-dev.com/job/gitpod-build-wth-test.176/raw
This commit creates a phase that has already been created: https://werft.gitpod-dev.com/job/gitpod-build-wth-test.178/raw

Release Notes

None

Documentation

ArthurSens

Nice improvements, looking forward to making slices and phases more consistent 🙂

I've made one first review round and found a couple of things that look a bit confusing. If others could also provide some feedback here, that would be awesome

ArthurSens · 2022-04-05T08:24:42Z

.werft/jobs/build/job-config.ts

+    werft.done("job config");
    werft.done(sliceId)


Looks super weird to see two slices being closed together at the moment 🤔

This could be avoided if the above code would use sliceId for logging instead of "job config".

.werft/util/werft.ts

ArthurSens · 2022-04-05T08:50:36Z

.werft/util/werft.ts

+        // Create a new slice entry that is already closed if no prior entry exists
+        if (!this.phases[this.currentPhase].slices[slice]) {
+            this.phases[this.currentPhase].slices[slice] = { "closed": true };
+        } else if (this.phases[this.currentPhase].slices[slice].closed) {
+            throw new Error(`The slice "${slice}" has already been closed!`);
+        } else {
+            this.phases[this.currentPhase].slices[slice].closed = true;
+        }


could this be split into a separated method?

I'm still struggling to understand what is the goal here 😬

It does happen that a new slice is opened with werft.done(). This should make sure that a slice is registered although it is closed directly and that a slice is not closed multiple times. Another way would be to enforce opening a new slice with werft.slice(<NAME>).

werft.done() opens a new slice? That doesn't seem right... I expect that a slice is only opened once we log something, or am I missing something here? 🤔

A good thing about splitting this into a new method is that we can keep methods small, and also give names to them. If we can't give a good name to a method, it probably means that it is too complex and could be made simpler 😅

fullmetalrooster · 2022-04-05T09:22:54Z

I can still see missing traces [1]. I take a deeper look and work on ending phases explicitly.

meysholdt · 2022-04-08T15:03:10Z

.werft/util/werft.ts

    private globalSpanAttributes: SpanAttributes = {}

+    public phases: { [name: string]: { "span": Span, "closed": boolean, "slices": { [name: string]: { "closed": boolean, "span": Span } } } } = {}


Can you introduce a Type for this datastructure?

I can do that. I have already proposed that in the issue [1].

I created a light version of my datastructures for phases and slices to solve this.

.werft/util/werft.ts

meysholdt · 2022-04-08T15:22:52Z

.werft/util/werft.ts

-            span.end()
-            delete this.sliceSpans[id]
-        })
+    public endPhase(name?: string) {


What's the use case for supporting name as a parameter?

It seems wrong because if I'm not mistaken only on phase can be open at a given time. This implies that this.currentPhase is always the phase that's being closed.

Adding the name explicitly would make the code more readable and to which phase the current code belongs.

.werft/jobs/build/prepare.ts

fullmetalrooster · 2022-04-11T07:46:13Z

I just ran a build on this PR. I get this warning at the end. Can you fix that?
WARNING: The slice "observability" has not been closed!
I ran the build --with=vm=true and got a broken trace. But that should not block this PR.

That is the reason why we moved from an error that breaks the deployment process to a warning [1].

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>

stale · 2022-04-23T13:08:15Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2022-05-25T23:40:52Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

roboquat added the do-not-merge/release-note-label-needed label Apr 1, 2022

fullmetalrooster marked this pull request as draft April 1, 2022 11:28

roboquat added do-not-merge/work-in-progress size/S size/M and removed size/S labels Apr 1, 2022

fullmetalrooster force-pushed the wulfthimm/werft-ts-should-throw-8707 branch 2 times, most recently from 2caaedc to 174a93f Compare April 1, 2022 14:19

roboquat added size/S size/M release-note-none and removed size/M size/S do-not-merge/release-note-label-needed labels Apr 1, 2022

fullmetalrooster force-pushed the wulfthimm/werft-ts-should-throw-8707 branch from 2476559 to 80de070 Compare April 4, 2022 06:44

fullmetalrooster marked this pull request as ready for review April 4, 2022 06:45

roboquat removed the do-not-merge/work-in-progress label Apr 4, 2022

fullmetalrooster force-pushed the wulfthimm/werft-ts-should-throw-8707 branch 2 times, most recently from 3ce536e to 3d51b8a Compare April 4, 2022 06:52

fullmetalrooster requested a review from a team April 4, 2022 07:05

github-actions bot added the team: devx label Apr 4, 2022

fullmetalrooster force-pushed the wulfthimm/werft-ts-should-throw-8707 branch 6 times, most recently from dee0df7 to 66c200b Compare April 4, 2022 08:54

ArthurSens reviewed Apr 5, 2022

View reviewed changes

meysholdt self-requested a review April 5, 2022 09:16

meysholdt reviewed Apr 8, 2022

View reviewed changes

.werft/util/werft.ts Outdated Show resolved Hide resolved

meysholdt reviewed Apr 8, 2022

View reviewed changes

.werft/util/werft.ts Outdated Show resolved Hide resolved

meysholdt reviewed Apr 8, 2022

View reviewed changes

.werft/util/werft.ts Outdated Show resolved Hide resolved

meysholdt reviewed Apr 8, 2022

View reviewed changes

.werft/jobs/build/prepare.ts Outdated Show resolved Hide resolved

fullmetalrooster force-pushed the wulfthimm/werft-ts-should-throw-8707 branch 4 times, most recently from 3d61060 to 1e39a70 Compare April 11, 2022 10:07

fullmetalrooster requested a review from meysholdt April 11, 2022 10:16

Wulf Thimm and others added 8 commits April 12, 2022 09:33

added validation tests to werft.ts

c2dea13

Update .werft/util/werft.ts

835ebad

Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>

fix

19623be

enforce phase ending

b22ff82

debug monitoring-satellite.ts

9f66bb2

log warnings in werft instead of errors

38371bd

fix

2089216

added extra classes for phases and slices

cf90a0b

fullmetalrooster force-pushed the wulfthimm/werft-ts-should-throw-8707 branch from 1e39a70 to cf90a0b Compare April 12, 2022 07:34

stale bot added the meta: stale This issue/PR is stale and will be closed soon label Apr 23, 2022

stale bot closed this Apr 28, 2022

meysholdt reopened this Apr 29, 2022

stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Apr 29, 2022

stale bot added the meta: stale This issue/PR is stale and will be closed soon label May 25, 2022

stale bot closed this May 31, 2022

mads-hartmann deleted the wulfthimm/werft-ts-should-throw-8707 branch September 14, 2022 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added validation checks for the werft API #9064

added validation checks for the werft API #9064

fullmetalrooster commented Apr 1, 2022 •

edited

Loading

ArthurSens left a comment

ArthurSens Apr 5, 2022

meysholdt Apr 8, 2022

ArthurSens Apr 5, 2022

fullmetalrooster Apr 5, 2022

ArthurSens Apr 5, 2022

fullmetalrooster commented Apr 5, 2022

meysholdt Apr 8, 2022

fullmetalrooster Apr 11, 2022 •

edited

Loading

fullmetalrooster Apr 11, 2022

meysholdt Apr 8, 2022

fullmetalrooster Apr 11, 2022 •

edited

Loading

fullmetalrooster commented Apr 11, 2022

stale bot commented Apr 23, 2022

stale bot commented May 25, 2022

		private globalSpanAttributes: SpanAttributes = {}

		public phases: { [name: string]: { "span": Span, "closed": boolean, "slices": { [name: string]: { "closed": boolean, "span": Span } } } } = {}

added validation checks for the werft API #9064

added validation checks for the werft API #9064

Conversation

fullmetalrooster commented Apr 1, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullmetalrooster commented Apr 5, 2022

Choose a reason for hiding this comment

fullmetalrooster Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullmetalrooster Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

fullmetalrooster commented Apr 11, 2022

stale bot commented Apr 23, 2022

stale bot commented May 25, 2022

fullmetalrooster commented Apr 1, 2022 •

edited

Loading

fullmetalrooster Apr 11, 2022 •

edited

Loading

fullmetalrooster Apr 11, 2022 •

edited

Loading