feat: report metrics for variants #150
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kwasniew
commented
May 4, 2023
| @@ -142,6 +156,19 @@ export default class Metrics { | |||
| return true; | |||
| } | |||
|
|
|||
| public countVariant(name: string, variant: string): boolean { | |||
There was a problem hiding this comment.
similar to Node.js implementation
kwasniew
commented
May 4, 2023
| @@ -217,6 +219,9 @@ export class UnleashClient extends TinyEmitter { | |||
| const enabled = toggle?.enabled || false; | |||
| const variant = toggle ? toggle.variant : defaultVariant; | |||
|
|
|||
| if (variant.name) { | |||
| this.metrics.countVariant(toggleName, variant.name); | |||
There was a problem hiding this comment.
variant is counted in addition to regular count for enabled/disabled
sjaanus
approved these changes
May 4, 2023
Tymek
added a commit
that referenced
this pull request
Jun 14, 2023
fix: Update tests (#147) This PR does three things: - Adds tests for undefined/null properties - Updates test titles - Formats the test suites The new tests were brought on by #146 I've added two (three) new tests. 1. `src/util.test.ts`: Verify that object properties set to `null` or `undefined` are excluded from query params 2. `src/index.test.ts`: Verify that setting a context field to `null` or `undefined` removes it from the context Two of the tests in `src/util.test.ts` seems to have had copy-paste mistakes in their titles. Looking at them, I believe the "not" in their descriptions should be removed. This repo has prettier listed in its deps, but the test files were pretty unformatted. My editor auto-formats on save, so this led to a lot of changes. However, I've isolated the formatting changes into separate commits, so we can easily cut those out if we don't want them. Diffing with **hidden whitespace** is encouraged. * fix: reword test names + change ts-ignore to ts-expect-error Looking at the tests, I can only assume that the "not" in their names was a copy-paste error. * format: format test suite * fix: add new test that checks null and undefined properties * fix: add test that clearing context works as expected * format: run prettier on tests * Fix: update ts-expect-error comment * add check for wrapped strings of null or undefined when filtering params * revert --------- Co-authored-by: andreas-unleash <andreas@getunleash.ai> fix: use custom headers for metrics requests too (#148) This PR updates how custom headers work. Previously, any custom headers you added would only get added to feature get/post requests. Now we also add them to metrics requests. This is in line with what the documentation implies and what we think it's reasonable to expect. As such, I consider this a bug fix. The most relevant changes are in the `src/metrics.ts`. In short, we perform the same kind of method here as in `src/index.ts` to enrich the standard headers with any custom headers that are provided. We also take care to pass the custom headers from the client constructor to the metrics constructor. The change comes with the following tests: 1. Verify that the custom headers are passed on from the client constructor to the metrics constructor 2. Verify that custom headers ... - are sent - override preset headers, but only if they have a valid value - that are set to `null` or `undefined` don't get sent. At the time of writing, the current set of changes are essentially just a copy-paste of what we were doing in the `index.ts` file. While it's tempting to refactor this and extract it, that also seems like a bit of overkill to me. As far as I'm aware, there's only two copies, and they behave slightly differently (different headers), so I think the duplication is accidental more than intentional. We could parameterize it into a shared function (and I'm happy to do that), but I'm not sure it's worth it. Second, we currently allow setting headers that are empty or all-whitespace strings. This is consistent with how the `index.ts` file does it. We can't change the index file, but we don't have to copy this behavior. Do we want to allow empty strings? I'm leaning towards yes because it's less surprising, but I'm open to hearing arguments. Finally, I also considered applying the custom headers to the `fetch` parameter that we send in (by wrapping it in another function). This would allow us to not change anything inside the `metrics.ts` file at all, but I decided against it because I thought it might be harder to reason about and to test. Again, though, happy to do it if we think it's better. --- Closes #142 * fix: add initial tests for custom headers * fix: add custom headers to metrics call * fix: test that custom headers are passed along * fix: pass custom headers to metrics * fix: elaborate on ts-expect-error message * docs: document how the sdk handles `null`/`undefined` Bump version to 2.4.4-beta.0 (#149) This commit prepares to release v2.4.4-beta.0 to release the custom header update in #148 . feat: report metrics for variants (#150) 2.4.4-beta.1 (#151) 2.5.0 (#154) build cjs prepack remove unused var add browser entrypoint chore: update ci (#156)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
About the changes
Main capability:
Changes:
Next steps:
Important files
Discussion points