## About the changes
We are losing some events because of not having "created by" which is
required by a constraint in the DB. One scenario where this can happen
is with the default user admin, because it doesn't have a username or an
email (unless configured by the administrator).
Our code makes assumptions on the existence of one of these 2 attributes
(e.g.
248118af7c/src/lib/services/user-service.ts (L220-L222)).
Event lost metrics:
## Discussion points
The solution proposed here is falling back to a default value. I've
chosen `"admin"` because it covers one of the use cases, but it can also
be `"system"` mimicking
248118af7c/src/lib/services/user-service.ts (L32)
which is used as a default, or `"unknown"` which is sometimes used as a
default:
248118af7c/src/lib/services/project-service.ts (L57)
Anyway, I believe it's better not to lose the event rather than be
accurate with the "created by" that can be fixed later
This PR adds potentially stale events as available to all addons and
adds a formatted message.
## Discussion
I'd still be interested in hearing input on whether the event type
should be selectable by addons or not before we start emitting the
event. I'm leaning towards "yes", but I'll take your thoughts into
consideration.
This PR does **one** thing:
it changes the events for potentially stale to:
- Only being emitted when potentially stale gets turned on
- In doing so, it also simplifies the event that's getting emitted,
removing the `data` property.
- The event is also renamed to better match the existing
`feature-stale-on` and `...-off` events.
The addon listening was broken out into a separate PR (#4279)
## Old description
This change lets all addons listen for events when features get marked
or unmarked as potentially stale.
### Discussion
#### All addons?
Should this be available to all addons? I can't see a reason why it
shouldn't be available to all addons, but I might be missing
something.
**Update**: spoke to a couple people. Can see no reason why this isn't
okay.
#### Should it be behind a flag?
The feature is still behind a flag, but the event type is not. Should
we gate the event being available until we actually emit the event?
That would require some more code, but could yield less potential
confusion.
Open to hearing your thoughts.
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
wrap reorder event creation to strategy variant feature
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
Fixes a bug around createStrategy
Fix: Create/Store strategy reorder event only when feature is on
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
This PR updates the feature type service by adding a new
`updateLifetime` method. This method handles the connection between the
API (#4256) and the store (#4252).
I've also added some new e2e tests to ensure that the API behaves as
expected.
This PR adds an operation and accompanying openapi docs for the new
"update feature type lifetime" API operation.
It also fixes an oversight where the other endpoint on the same
controller didn't use `respondWithValidation`.
Note: the API here is a suggestion. I'd like to hear whether you agree
with this implementation or not.
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
When reordering strategies for a feature environment:
- Adds stop when CR are enabled
- Emits an event
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
This PR activates the event emission that was prepared for in
https://github.com/Unleash/unleash/pull/4239.
It emits events (behind a flag) when something is marked as potentially
stale or the opposite.
It takes the features returned from the store and creates events out of
them.
The events only contain data, no preData. This is because the preData
can easily be inferred and because it gives a nicer event in the event
log.
Here is an image of the difference. The top event uses only data, so it
shows the name of the feature and the new potentiallyStale status. The
bottom event uses both preData and data, so it only shows the new
potentiallyStale status and not the feature name (unless you show the
raw event):
Should not be merged before #4239. Merge that and then rebase this off
main or cherry the commit.
## Discussion
### `preData`
Should we also use preData or is it enough to use only data? It seems
unnecessary in this event, but I'm open to hearing your thoughts.
### event author: `createdBy`
I've set `unleash-system` as the `createdBy` property on these events
because they are generated by the system. I found the same string used
some other places. However, it may be that there we want to use a
different author.
This PR adds updates the potentially stale status change events whenever
the potentially stale update function is run.
No events are emitted yet. While the emission is only a few lines of
code, I'd like to do that in a separate PR so that we can give it the
attention it deserves in the form of tests, etc.
This PR also moves the potentially stale update functionality from the
`update` method to only being done in the
`updatePotentiallyStaleFeatures` method. This keeps all functionality
related to marking `potentiallyStale` in one place.
The emission implementation was removed in
4fb7cbde03
## The update queries
While it would be possible to do the state updates in a single query
instead of three separate ones, wrangling this into knex proved to be
troublesome (and would also probably be harder to understand and reason
about). The current solution uses three smaller queries (one select, two
updates), as Jaanus suggested in a private slack thread.
This reverts commit 16e3799b9a.
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
https://linear.app/unleash/issue/2-1232/implement-first-iteration-of-the-new-slack-app-addon
This PR implements the first iteration of the new Slack App addon.
Unlike the old Slack addon, this one uses a Slack App (bot) that is
installed to Slack workspaces in order to post messages. This uses
`@slack/web-api`, which internally uses the latest Slack API endpoints
like `postMessage`.
This is currently behind a flag: `slackAppAddon`.
The current flow is that the Unleash Slack App is installed from
whatever source:
- Unleash addons page;
- Direct link;
- https://unleash-slack-app.vercel.app/ (temporary URL);
- Slack App Directory (in the future);
- Etc;
After installed, we resolve the authorization to an `access_token` that
the user can paste into the Unleash Slack App addon configuration form.
https://github.com/Unleash/unleash/assets/14320932/6a6621b9-5b8a-4921-a279-30668be6d46c
Co-authored by: @daveleek
---------
Co-authored-by: David Leek <david@getunleash.io>
This PR lays most of the groundwork required for emitting events when
features are marked as potentially stale by Unleash. It does **not**
emit any events just yet. The summary is:
- periodically look for features that are potentially stale and mark
them (set to run every 10 seconds for now; can be changed)
- when features are updated, if the update data contains changes to the
feature's type or createdAt date, also update the potentially stale
status.
It is currently about 220 lines of tests and about 100 lines of
application code (primarily db migration and two new methods on the
IFeatureToggleStore interface).
The reason I wanted to put this into a single PR (instead of just the db
migration, then just the potentially stale marking, then the update
logic) is:
If users get the db migration first, but not the rest of the update
logic until the events are fired, then they could get a bunch of new
events for features that should have been marked as potentially stale
several days/weeks/months ago. That seemed undesirable to me, so I
decided to bunch those changes together. Of course, I'd be happy to
break it into smaller parts.
## Rules
A toggle will be marked as potentially stale iff:
- it is not already stale
- its createdAt date is older than its feature type's expected lifetime
would dictate
## Migration
The migration adds a new `potentially_stale` column to the features
table and sets this to true for any toggles that have exceeded their
expected lifetime and that have not already been marked as `stale`.
## Discussion
### The `currentTime` parameter of `markPotentiallyStaleFeatures`
The `markPotentiallyStaleFetaures` method takes an optional
`currentTime` parameter. This was added to make it easier to test (so
you can test "into the future"), but it's not used in the application.
We can rewrite the tests to instead update feature toggles manually, but
that wouldn't test the actual marking method. Happy to discuss.
## About the changes
Fix un-awaited promise on batch variant update - reduce function allowed
TS to skip Promise type.
---------
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
<!-- Thanks for creating a PR! To make it easier for reviewers and
everyone else to understand what your changes relate to, please add some
relevant content to the headings below. Feel free to ignore or delete
sections that you don't think are relevant. Thank you! ❤️ -->
Wraps the whole `registerClientMetrics` function with try/catch to
return 400 on error
## About the changes
<!-- Describe the changes introduced. What are they and why are they
being introduced? Feel free to also add screenshots or steps to view the
changes if they're visual. -->
<!-- Does it close an issue? Multiple? -->
Closes #
[1-1037](https://linear.app/unleash/issue/1-1037/return-4xx-error-for-incorrect-metrics-input)
<!-- (For internal contributors): Does it relate to an issue on public
roadmap? -->
<!--
Relates to [roadmap](https://github.com/orgs/Unleash/projects/10) item:
#
-->
### Important files
<!-- PRs can contain a lot of changes, but not all changes are equally
important. Where should a reviewer start looking to get an overview of
the changes? Are any files particularly important? -->
## Discussion points
<!-- Anything about the PR you'd like to discuss before it gets merged?
Got any questions or doubts? -->
---------
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
## About the changes
- Adding descriptions and examples to tag and tag types schemas
- Adding standard errors, summaries, and descriptions to tag and tag
types endpoints
- Some improvements on compilation errors
---------
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
Make each error class have to define its own status code. This makes
it easier for developers to see which code an error corresponds to and
means less jumping back and forth between files. In other words:
improved locality.
Unfortunately, the long switch needs to stay in case we get errors
thrown that aren't of the Unleash Error type, but we can move it to
the `fromLegacyError` file instead.
Tradeoff analysis by @kwasniew:
+ I like the locality of error to code reasoning
- now HTTP leaks to the non-HTTP code that throws those errors e.g. application services
If we had other delivery mechanisms other than HTTP then it wouldn't make sense to couple error codes to one protocol (HTTP). But since we're mostly doing web it may not be a problem.
@thomasheartman's response:
This is a good point and something I hadn't considered. The same data was always available on those errors (by using the same property), I've just made the declaration local to each error instead of something that the parent class handles. The idea was to make it easier to create new error classes with their corresponding error codes. Because the errors are intended to be API errors (or at least, I've always considered them to be that), I think that makes sense.
Taking your comment into consideration, I still think it's the right thing to do, but I'm not bullish about it. We could always walk it back later if we find that it's not appropriate. The old code is still available and we could easily enough roll back this change if we find that we want to decouple it later.
